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

Reimplement OpenBabel::OBRandom as a wrapper of std::mt19937_64 #2241

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

e-kwsm
Copy link
Contributor

@e-kwsm e-kwsm commented May 25, 2020

Mersenne twister is one of high-quality PRNGs and available on all platforms since C++11.

@e-kwsm e-kwsm force-pushed the mt19937_64 branch 2 times, most recently from a4075bd to f22042d Compare May 25, 2020 07:11
@ghutchis
Copy link
Member

Looks like a good idea.

One thing that's been requested to add an OBOp plugin to set the RNG seed (e.g., #1934) - would you be willing to add that too.

@baoilleach
Copy link
Member

I've said it before but we should be working to remove all uses of random numbers in the library.

@ghutchis
Copy link
Member

@baoilleach - I don't think that's possible. It's good to minimize them, allow a defined seed to be set, etc. but stochastic methods are critical to several tasks in the library.

@ghutchis
Copy link
Member

ghutchis commented Aug 3, 2022

We've generally said that only major version changes (e.g., OB2 => OB3 or OB3 => OB4) would drop functions / methods.

The general concept is great, but can we either keep the existing code for backwards compatibility and add a new OBRandomMT class to use in conformer search, etc.? The other alternative would be to replace with the current code but wrap previous methods for backwards compatibility.

Otherwise we have to leave this for a while.

@e-kwsm e-kwsm force-pushed the mt19937_64 branch 2 times, most recently from f62b547 to 3f82d3a Compare August 17, 2022 02:32
@e-kwsm
Copy link
Contributor Author

e-kwsm commented Aug 17, 2022

Done.

At the moment, i.e. OB_VERSION < OB_VERSION_CHECK(4, 0, 0), the codes still use OBRandom and thus are backward compatible (rounding error may occur).
They will be automatically changed to utilize OBRandomMT when version 4 is released.
Explanations of OB_RANDOM_SEED environment variable in man docs are currently commented out.

@alexander-telepov
Copy link

alexander-telepov commented Sep 26, 2022

@e-kwsm thanks for such opportunity to fix seeds for reproducibility! However after build this pr I still can't repeat results when call obabel from bash, both putting seed argument in and after setting environment variable, for example for this CC1=CC(CNC2=CC=C(NC3CCC4=CC=C(C5=NNN=N5)C=C43)C=C2NC2=CC=C(F)N=C2F)=NC(C)=N1 mol. I build library with 3 option from here. Could you please suggest some workaround?

@e-kwsm
Copy link
Contributor Author

e-kwsm commented Sep 28, 2022

@alexander-telepov You have to build Open Babel with -DOB_USE_OBRANDOMMT to enable the feature.

@alexander-telepov
Copy link

@e-kwsm Thanks for such fast reply!

this is enabled if OB_USE_IMPROVED_RANDOM_UNIT_VECTOR, which defaults to
whether OpenBabel 4 is released or not, is defined as truthy
the macro defaults to whether OpenBabel 4 is released or not
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