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

Add seed fixing option to PySMO's sampling methods to enhance reproducibility #1307

Merged
merged 30 commits into from
Jan 25, 2024

Conversation

OOAmusat
Copy link
Contributor

Summary/Motivation:

This PR adds the ability for users to specify a seed with rand_seed when using the LatinHypercubeSampling, CVTSampling and CustomSampling classes in PySMO. Fixing rand_seed ensures that the sampling methods return the same results every time the methods are run, allowing for reproducibility.

Changes proposed in this PR:

  • Add rand_seed argument to CVT, LHS and custom sampling methods
  • Add tests

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

Copy link

codecov bot commented Dec 18, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f07552d) 77.41% compared to head (2477b3e) 77.41%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1307   +/-   ##
=======================================
  Coverage   77.41%   77.41%           
=======================================
  Files         390      390           
  Lines       63631    63649   +18     
  Branches    11702    11705    +3     
=======================================
+ Hits        49258    49273   +15     
- Misses      11835    11839    +4     
+ Partials     2538     2537    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@andrewlee94 andrewlee94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few suggestions

idaes/core/surrogate/pysmo/sampling.py Outdated Show resolved Hide resolved
@@ -1412,6 +1422,12 @@ def __init__(
raise Exception("Invalid tolerance input")
self.eps = tolerance

if rand_seed is not None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing as this is in __init__, should this code be moved to the base class to avoid duplicating the code in each subclass?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I'd rather leave it here because it is an argument specific to a subset of the sampling methods.

@ksbeattie ksbeattie added the Priority:Normal Normal Priority Issue or PR label Dec 21, 2023
@ksbeattie ksbeattie added the CI:run-integration triggers_workflow: Integration label Jan 11, 2024
@idaes-build idaes-build removed the CI:run-integration triggers_workflow: Integration label Jan 11, 2024
@ksbeattie ksbeattie added the CI:run-integration triggers_workflow: Integration label Jan 11, 2024
@idaes-build idaes-build removed the CI:run-integration triggers_workflow: Integration label Jan 11, 2024
@dangunter dangunter self-requested a review January 18, 2024 19:40
Copy link
Member

@dangunter dangunter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@dangunter dangunter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ksbeattie ksbeattie merged commit 7ee5489 into IDAES:main Jan 25, 2024
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority:Normal Normal Priority Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants