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

Expose seed setting capability in kmedoids. #1

Merged
merged 2 commits into from
Dec 11, 2014

Conversation

kisaacs
Copy link
Member

@kisaacs kisaacs commented Dec 8, 2014

Added a function to set the random seed so I can keep track of what it is and reproduce results.

@tgamblin
Copy link
Member

tgamblin commented Dec 9, 2014

@kisaacs: This looks ok, but did you look at how the parallel algorithms set their seeds? They rely on having the same seed on each process, and they bcast it out on every invocation of capek() and xcapek(). So this setter won't actually work for the parallel algorithms.

I think the right way to address that is to record in kmedoids whether the user overrode the seed, and have the parallel algorithms take the user's value from rank 0 and bcast it if he did. Of course, that could confuse things and get you a deadlock if the user only calls set_seed() on one rank. In which case you might just skip the bcast and document that the user needs to call set_seed() with the same value on all ranks or it's an error, which is more in the spirit of MPI and it's the user's fault if he screws up.

Thoughts?

@kisaacs
Copy link
Member Author

kisaacs commented Dec 9, 2014

@tgamblin It seemed different enough in spirit that I didn't attempt to insert a corresponding set_seed in par_kmedoid. To make them similar, I would move the time/bcast seed setting to the constructor, remove it from capek/xcapek and then the user can set_seed as you suggested without recording whether the seed was overriden. This change should be transparent to users who don't want to explicitly set a seed. Sound good?

@kisaacs
Copy link
Member Author

kisaacs commented Dec 10, 2014

Oh wait, looking more closely at how the par_ version is used, I see that won't work and yes we need to record whether the seed has been set.

@tgamblin
Copy link
Member

Looks good to me. Thanks!

tgamblin added a commit that referenced this pull request Dec 11, 2014
Expose seed setting capability in kmedoids.
@tgamblin tgamblin merged commit b58796b into LLNL:master Dec 11, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants