-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Make @threads work on array comprehensions
#59019
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
fea28a0 to
d2bced7
Compare
|
the fact that this always returns a just some further cases that might want to be handled: multiple loops non-indexable iterators |
|
it seems a little odd to me that Could it be called (BTW: feels like there might be some similarity to |
|
If |
d2bced7 to
94385ff
Compare
|
@adienes I addressed your points in #59019 (comment) @ericphanson I kept |
94385ff to
1e8b21d
Compare
1e8b21d to
fb9f706
Compare
fb9f706 to
bcfc617
Compare
| @test all(result_greedy[i] == i^2 for i in 1:n) | ||
| @test issorted(result_greedy) # should be ordered for greedy scheduling too |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two tests seem wrong. :greedy does not guarantee any particular execution order, so there is no reason it should be preserved. It's better to check that every element in the result is also present when the same kind of collection is done without threading:
| @test all(result_greedy[i] == i^2 for i in 1:n) | |
| @test issorted(result_greedy) # should be ordered for greedy scheduling too | |
| result_nonthreaded = [ i^2 for i in 1:n ] | |
| @test all(result_greedy) do r | |
| r in result_nonthreaded | |
| end |
This is of course quadratic, but since :greedy doesn't guarantee an order, anything else runs into ordering problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But now we store i greedy has been made order-preserving via the sort at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:greedy should not have to guarantee that though. The suggestion for adding the original index I gave on slack was for :static and :dynamic, which are order preserving by design.
| @test length(result_greedy) == length(expected) | ||
| @test result_greedy == expected # should preserve order |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, :greedy does not guarantee this.
| result_greedy = @threads :greedy [i * j for i in 1:3, j in 1:3] | ||
| @test size(result_greedy) == size(expected_static) | ||
| @test result_greedy == expected_static # greedy scheduling preserves order and dimensions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
Closes #209
See docstring for examples.
Developed with help from Claude.