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

Fix random seed on tests #2844

Merged
merged 3 commits into from
Apr 17, 2023
Merged

Fix random seed on tests #2844

merged 3 commits into from
Apr 17, 2023

Conversation

jsbrittain
Copy link
Contributor

Description

Some tests in the unit and integration suites call functions that make use of random variables, or include random variables in their calls (specifically numpy.random). This can lead to tests sometimes failing due to reasons that may be unrelated to recent code changes, as outlined (and demonstrated) in issue #2833.

Fixes #2833

Type of change

Fixing the numpy random seed within the test routines produces consistent behavior. However, since stochastic elements are present in the PyBaMM codebase in relation to both the Casadi and Idaklu solvers, extensive dependencies on these functions within the test suite are likely to be widespread and opaque. In order to fully rectify the issue (whilst maintaining the same simplicity in test set-up moving forward) a metaclass solution is proposed.

The proposed solution wraps all methods that inherit from TestCase with a wrapper function that fixes the random seed to a hash code derived from the method's name. This permits hashes to be changed in problematic circumstances by a hash modifier, such as appending a trailing underscore (or other modifier) to the method name.

Specifically, the TestCase class, which is described as the 'Custom TestCase class for pybamm' is given a custom metaclass. However, it was also noted that this test class is not in widespread use across the testbase, and hence unittest.TestCase has now been replaced with TestCase as imported from tests across all test scripts.

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.

  • New feature (non-breaking change which adds functionality)
  • Optimization (back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)

Key checklist:

  • No style issues: $ pre-commit run (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
  • All tests pass: $ python run-tests.py --all
  • The documentation builds: $ python run-tests.py --doctest

You can run unit and doctests together at once, using $ python run-tests.py --quick.

Further checks:

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@codecov
Copy link

codecov bot commented Mar 31, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (cc1bff5) 99.69% compared to head (5fa9e97) 99.69%.

❗ Current head 5fa9e97 differs from pull request most recent head 6f1a384. Consider uploading reports for the commit 6f1a384 to get more accurate results

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #2844   +/-   ##
========================================
  Coverage    99.69%   99.69%           
========================================
  Files          273      273           
  Lines        19065    19065           
========================================
  Hits         19006    19006           
  Misses          59       59           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jsbrittain jsbrittain marked this pull request as ready for review March 31, 2023 19:25
Copy link
Member

@valentinsulzer valentinsulzer left a comment

Choose a reason for hiding this comment

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

Looks good to me. Will need changelog update (moving to new section) and merging the changes from the casadi 3.6 update

@valentinsulzer
Copy link
Member

Changelog updates are in the wrong month here

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.

[Bug]: Some integration tests are non-deterministic
3 participants