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

Improve reliability of simulations when using CasadiSolver #3494

Merged

Conversation

agriyakhetarpal
Copy link
Member

@agriyakhetarpal agriyakhetarpal commented Nov 2, 2023

Description

Closes #3415; wraps and calls a fixed random seed set to the name of the Simulation class when using the CasadiSolver or CasadiAlgebraicSolver solvers for a model, which are susceptible to the initial conditions and sometimes fail randomly. This situation has been fixed for the integration tests and for the benchmarks earlier, this PR ensures that it will be fixed for general-purpose use-cases of the Simulation class.

Setting a random seed has been shown to bring the failure rate to zero on repeated executions of the half-cell models Jupyter notebook: https://github.com/agriyakhetarpal/PyBaMM/actions/runs/6709408022, https://github.com/agriyakhetarpal/PyBaMM/actions/runs/6729021051.

Type of change

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 (or $ nox -s pre-commit) (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 (or $ nox -s tests)
  • The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s quick).

Further checks:

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

@agriyakhetarpal
Copy link
Member Author

agriyakhetarpal commented Nov 2, 2023

Since this is a bug fix, should this be included in the release? We had decided to look into the issue in the next one, but I think this should be conclusive enough and is also a small change

Copy link

codecov bot commented Nov 2, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2ee8da2) 99.58% compared to head (dfde5d0) 99.58%.
Report is 106 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3494   +/-   ##
========================================
  Coverage    99.58%   99.58%           
========================================
  Files          256      256           
  Lines        20126    20131    +5     
========================================
+ Hits         20043    20048    +5     
  Misses          83       83           

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

Copy link
Contributor

@kratman kratman left a comment

Choose a reason for hiding this comment

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

Using a hash of the function name seems to be a bit of overkill, but it looks good. The failing integration tests look like they are just a sporadic pip download issue

@agriyakhetarpal
Copy link
Member Author

Using a hash of the function name seems to be a bit of overkill, but it looks good.

I agree, albeit setting it as such should ensure that the hash will change at a time only when the name of the class changes—which will never happen

Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

I can include this in the next rc release once it is merged in develop.

Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

Thanks, @agriyakhetarpal! I'll merge once the conflicts are resolved.

@Saransh-cpp Saransh-cpp merged commit d7d6262 into pybamm-team:develop Nov 25, 2023
33 of 35 checks passed
@agriyakhetarpal agriyakhetarpal deleted the random-seed-for-simulations branch November 25, 2023 15:59
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.

Scheduled tests failing while running example tests on python 3.8 (Ubuntu)
3 participants