Skip to content
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

Random's iterate() method: replace with iterator these()? #19603

Closed
vasslitvinov opened this issue Apr 5, 2022 · 10 comments
Closed

Random's iterate() method: replace with iterator these()? #19603

vasslitvinov opened this issue Apr 5, 2022 · 10 comments

Comments

@vasslitvinov
Copy link
Member

vasslitvinov commented Apr 5, 2022

see also / is this a duplicate of #10717 ?

Can/should we replace PCGRandomStream.iterate() method with the iterator these()? Having a these iterator is more natural.

Background: iterate() yields a random number for every index in a domain that's passed to it as an argument. iterate(D) advances the generator state by D.size immediately, before yielding any values. If D is distributed, each locale/task effectively gets its own private copy of the random number generator that starts at an appropriate offset.

In other words, iterate() is like fillRandom(), except it yields instead of updating an array element. In fact, fillRandom(A) is implemented by zippering iterate(A.domain) with A.

Ideally, instead of forall i in iterate(D) we would like to write forall (_, i) in zip(D, myRNG). How to accomplish this? What leader/follower improvements, if any, does this require?

@vasslitvinov
Copy link
Member Author

For now, we should mark iterate() as unstable.

@bradcray
Copy link
Member

bradcray commented Apr 5, 2022

How to accomplish this? What leader/follower improvements, if any, does this require?

I agree that this issue's proposal is attractive, but believe it requires the ability to have follower iterators be able to know the total number of elements being iterated over rather than just their place in it so that they can advance the cursor. It may also require some sort of "We're all starting now" or "We're all done now" call so that the cursor can be advanced at a safe point (i.e., that other followers aren't going to read the old/new value at the wrong time). I view this as being fairly similar to #11428

@mppf
Copy link
Member

mppf commented Apr 7, 2022

I think a more modest (and hopefully easy to achieve) proposal would be to simply make proc iterate() into iter iterate() that is a leader/follower iterator. That would make it use less undocumented pragmas, etc.

We could certainly create a iter these(D: domain) but I am uncertain of the value of such a thing (basically, since it takes a required argument, the compiler can't call it, so why is it called these?). I think it is more work to get leader/follower improvements to get the zipper case working without passing the domain. So I lean towards simply having iter iterate(D: domain) in the near term.

Of course I think it is good to do those leader/follower improvements - I am just not so sure we want to tie the Random module stability to them.

@bradcray
Copy link
Member

bradcray commented Apr 7, 2022

Unless I've missed something, I don't think an iter iterate(D: domain) can serve as a follower because there's no obvious / easy place to advance the cursor safely? e.g., you're asked to yield elements 0..3 from the current cursor and I'm asked to yield elements 4..7. Which of us bumps the shared cursor up by 8 and how do we know when it's safe to do so?

@mppf
Copy link
Member

mppf commented Apr 7, 2022

Good point. I think it would work if the iter iterate was the leader, though, right?

@bradcray
Copy link
Member

bradcray commented Apr 7, 2022

Yeah, that's definitely the easier case (e.g., bump the cursor by the number of indices after all the tasks the leader created have completed).

@mppf
Copy link
Member

mppf commented Apr 8, 2022

Riffing on the context manager, we could have loops (including zippered loops) call enterThese and leaveThese on the iterated things.

@lydia-duncan
Copy link
Member

Random is no longer intended for 2.0, removing the tag

@jeremiah-corrado
Copy link
Contributor

Making a note that #24379 replaced iterate with another iterator method called next.

In stabilization discussions, there was still a desire to eventually support an iter these in addition to next; however, as Brad pointed out above, that will require some improvements to the existing parallel iterator design:

I agree that this issue's proposal is attractive, but believe it requires the ability to have follower iterators be able to know the total number of elements being iterated over rather than just their place in it so that they can advance the cursor.

@jeremiah-corrado
Copy link
Contributor

Closing in favor of #10717, since this is essentially a duplicate issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants