search

LLNL / ygm

Other
31 stars 22 forks source link

adding `local_for_random_samples()` function to containers that randomly selects a local subset of objects to be passed to a lambda #124

Closed bwpriest closed 1 year ago

bwpriest commented 1 year ago

Leaving as a draft until #123 is merged, as this will need to be changes slightly to conform.

steiltre commented 1 year ago

I think we should talk a little more about the interface for this feature. The name for_some suggests to me that it applies to some subset of elements that meet some filtering criteria, rather than random sampling. for_random_samples might be more appropriate.

I also think we should prefix the name with local_ to signify that the sampling is being done over an individual rank's shard of the container. This naming would match the ygm::container::map::local_get() pattern and would leave room for us to add global sampling in the future.

My final change would be to create a version of this functionality that allows a user to pass in the random number generator.

steiltre commented 1 year ago

Oh yeah. PR #123 has been merged in now.

bwpriest commented 1 year ago

I think we should talk a little more about the interface for this feature. The name for_some suggests to me that it applies to some subset of elements that meet some filtering criteria, rather than random sampling. for_random_samples might be more appropriate.

That makes sense. I was avoiding the for_random_samples name because I wanted to avoid the implication that the samples were uniformly sampled without replacement from the whole container.

I also think we should prefix the name with local_ to signify that the sampling is being done over an individual rank's shard of the container. This naming would match the ygm::container::map::local_get() pattern and would leave room for us to add global sampling in the future.

This addresses my initial concern. local_random_sample sounds good to me.

My final change would be to create a version of this functionality that allows a user to pass in the random number generator.

Oh yeah. I'll add that functionality.

bwpriest commented 1 year ago

@steiltre I believe that I have addressed your concerns and merged with develop. I've also made sure that the new functionality supports both local map lambda signatures.

bwpriest commented 1 year ago

This functionality will be dependent upon ygm::random to be defined per iss #131. I'm marking it as a draft until then.

bwpriest commented 1 year ago

I seriously broke the commit history of this one, so I am going to throw the PR away and start over.