Fixing inconsistent results when using seed in examples (reopened) #3621
Conversation
|
Continuation of PR # 3608. Changed the branch name to be more consistent with goal, did not realize that would close PR. Thank you @blnicho for feedback and approval of change. Will make requested changes, make sure tests don't break, and change from draft to full PR. |
|
@adowling2 @djlaky Ready for review, changes made examples reproducible with a set seed. Seeds also added to test_file. Test results: |
| print(lNo_theta.head()) | ||
|
|
||
| parmest.graphics.pairwise_plot(lNo_theta, theta) | ||
| parmest.graphics.pairwise_plot(lNo_theta, theta, seed=524,) |
|
|
||
|
|
||
| if SET_GLOBAL_SEED: |
There was a problem hiding this comment.
Alternate idea: either here or in the examples file (or somewhere else the Pyomo team recommends), define
_RANDOM_SEED_FOR_TESTING = 524
Then in all the files that use this random seed, you can import this value.
Then in this file:
if _RANDOM_SEED_FOR_TESTING:
np.random.seed(_RANDOM_SEED_FOR_TESTING)
This will mean someone only sets the testing random seed in one spot.
|
Plan for this week: |
Notes for Pyomo Dev Meeting 6/17/25:
Context Problem Implemented solution Questions
|
|
@sscini, I think the only documentation needed will be to explain any new function arguments (if applicable). My understanding is that this PR primarily fixes tests, so I don't think it will require any new tests. |
|
Update: Now regular PR, all tests pass. Planning to change seed to rng to fit new supported scipy and numpy formatting. |
| @@ -33,6 +33,10 @@ | |||
| pynumero_ASL_available = AmplInterface.available() | |||
| testdir = this_file_dir() | |||
|
|
|||
| # Set the global seed for reproducibility | |||
| _RANDOM_SEED_FOR_TESTING = 524 | |||
| np.random.seed(_RANDOM_SEED_FOR_TESTING) # Set seed for reproducibility | |||
There was a problem hiding this comment.
You'll probably need to put a try ... except around this. You are now making numpy a hard requirement.
There was a problem hiding this comment.
This is a good suggestion.
@sscini Do the bootstrap, leave_n_out, ... functionality require numpy or scipy?
There was a problem hiding this comment.
@adowling2 Agreed, I will add a try, except. Bootstrap and leave_N_out rely on numpy, graphics.pairwise_plot, and other graphics functions use scipy.
There was a problem hiding this comment.
@mrmundt Would the "except" in this case be an import error for numpy? The other option would be adding the seed within each of the test classes, but if the try except works that's a lot less additions.
There was a problem hiding this comment.
I think, considering that the random seed is necessary for these tests to run, that it would be best to do something like:
if numpy_available:
np.random.seed(...)
else:
raise unittest.SkipTest('Numpy is not available')
You can import numpy_available in line 25:
from pyomo.common.dependencies import numpy as np, numpy_available, pandas as pd, scipy, matplotlib
Caveat: This will make ALL parmest tests unavailable, so don't do that if you still want some tests to run.
Fixes # .
When using the reactor_design leave_N_out example in parmest, the results from multiple runs of the example vary a lot, with random seed defined.
Summary/Motivation:
Using a random seed should allow for reproducible results. Currently, the results vary in the leave_N_out reactor design example (pyomo/pyomo/contrib/parmest/examples/reactor_design/leaveNout_example.py) for parmest even with an active random seed. I included a temporary .py file (pyomo/pyomo/contrib/parmest/examples/reactor_design/mwe_bootstrap_error_SSE.py) with the minimum working example showing the results vary with multiple runs.
Changes proposed in this PR:
TODO before converting from draft:
Legal Acknowledgement
By contributing to this software project, I have read the contribution guide and agree to the following terms and conditions for my contribution: