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

Setting RNG in StratifiedCrossValidationSplitting for thread safety #1424

Merged
merged 5 commits into from Aug 23, 2013

Conversation

tklein23
Copy link
Contributor

Using the same CRandom instance over multiple threads is not safe. Modified StratifiedCrossValidationSplitting so that we can set own RNGs using member m_rng. This patch works for me when using different instances of CStratifiedCrossValidationSplitting across multiple threads.

@karlnapf: I think there's need for discussion. Please have a look.

@karlnapf
Copy link
Member

Nice!

We should update unit tests though if we touch this. See the libshogun examples where I wrote test examples which can be migrated.
Also , please also update the other x validation splitting and add a note in the class description that it is thread safe random number generator.

Sorry for all the requests and thanks again;)

@tklein23
Copy link
Contributor Author

I updated my PR and shuffle(CRandom *) methods to several classes. I found out that the parameter-less shuffle() is not consumed somewhere else, so I encourage to remove it. Objections?

@karlnapf
Copy link
Member

agreed!

What about the examples/test? Interested? :)

@sonney2k
Copy link
Member

Yes please keep the static one too.

@tklein23
Copy link
Contributor Author

@sonney2k: Reverted my last commit, so your parameter-less (and non-thread-safe) methods are back.

@karlnapf: I know good test coverage is important, but I currently can't do it. Have a brief look on the complete diff. I think unit-tests are not (strongly) required for the following reasons:

  • I simplified my changes so much that they only rely on modified versions of "permute()" or "shuffle()".
  • I added a note to "shuffle()" that it's not thread safe
  • Since m_rng is initialized with "sg_rand" by default, the behaviour did not change (unless you explicitly set m_rng)

@karlnapf
Copy link
Member

SGVectorTest.dot failed on travis?

@karlnapf
Copy link
Member

@tklein23 I agree it is safe to not add them. Though it would be very easy since the tests already exist as c++ examples, just have to be translated to unit tests. But I can do that also at some point.

@tklein23
Copy link
Contributor Author

I did a rebase & push --force to trigger travis. It's (almost) green now.

@iglesias
Copy link
Collaborator

It is possible to restart complete builds or jobs from the travis GUI. You have to sign in with your github account and I guess that permissions for the repository are also needed.

@tklein23
Copy link
Contributor Author

@iglesias: Thanks for telling. Didn't know that.

karlnapf added a commit that referenced this pull request Aug 23, 2013
Setting RNG in StratifiedCrossValidationSplitting for thread safety
@karlnapf karlnapf merged commit 021393b into shogun-toolbox:develop Aug 23, 2013
@tklein23 tklein23 deleted the thread_safe_xvalidation branch December 10, 2013 23:59
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.

None yet

4 participants