-
Notifications
You must be signed in to change notification settings - Fork 586
Fixing inconsistent results when using seed in parmest examples #3621
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
Merged
Merged
Changes from 18 commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
e9308d2
Added locations of fixes needed.
sscini 34849d7
Merge branch 'Pyomo:main' into fix-SSE-w-all-theta_est
sscini effdc8c
Create mwe_bootstrap_error_SSE.py
sscini 82b4a7e
Moved temp error file to correct folder
sscini 3c97998
Update mwe_bootstrap_error_SSE.py
sscini 72e9974
Commented out plotting code for output simplicity.
sscini b1e1181
Renamed example files, and added potential solution to parmest.py
sscini d268613
Update parmest.py
sscini ab2e67b
Update for discussion in dev meeting
sscini e75f03b
Merge branch 'Pyomo:main' into fix-SSE-w-all-theta_est
sscini ce310ef
Added seed to 3 examples, removed temp.py files
sscini 279036f
Removed extra spaces
sscini f0d74d5
Seed is now available option for all random generators I could find
sscini 1fbe06c
Confirmed tests run with changes
sscini c19d59f
Ran black on parmest folder
sscini 0da9794
Merge branch 'Pyomo:main' into Example-seed-issue
sscini a1c1073
Update test_parmest.py
sscini f760b8f
Merge branch 'Pyomo:main' into Example-seed-issue
sscini ab30261
Update test_parmest.py
sscini 466e3b4
Ran black, all tests pass, final lookover
sscini d7db8d3
Merge branch 'main' into Example-seed-issue
blnicho File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll probably need to put a
try ... exceptaround this. You are now makingnumpya hard requirement.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good suggestion.
@sscini Do the bootstrap, leave_n_out, ... functionality require numpy or scipy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, considering that the random seed is necessary for these tests to run, that it would be best to do something like:
You can import
numpy_availablein line 25:Caveat: This will make ALL parmest tests unavailable, so don't do that if you still want some tests to run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adowling2 Thoughts on this? Numpy is present in a lot of the tests here, so I think having the numpy_available would not be horrible to add.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would probably be best practice to explicitly set the random seed in only the tests that need it. This gets around the caveat that @mrmundt points out. It also makes it super clear to future developers which tests/features require numpy (or scipy).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adowling2 @mrmundt Thank you both for suggestions. So final action item I interpreted will be leaving the number (line 37) at the top of the file, and then adding the np.random.seed(...) to the tests that require it at the class level.