Fix a bug where the usage of the CasADi solver breaks reproducibility

[agriyakhetarpal]

Description

This is an issue that I highlighted during our presence at the PyBaMM conference around a week ago: in #3494 (which closed #3415), I introduced a bug wherein a NumPy random seed was set when using the CasadiSolver to solve a simulation. It is discouraged to add a random seed in library/module code – it breaks reproducibility by overriding randomness that may have been set by user-level code in researchers' scripts and simulations (it is acceptable for test code, though).

As the PR was supposed to fix stochastic behaviour for the hall-cell model, I have moved the random seed to the notebook instead which used to fail at that time in the repository's development. Usually, this should be present for all notebooks, but one should be enough for now to keep the diff minimal.

Another potential fix is to look at the globals() and locals() to find if a seed has been set already and use that, but that seems to be a bit over-engineered.

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #)

Important checks

Please confirm the following before marking the PR as ready for review:

• No style issues: nox -s pre-commit
• All tests pass: nox -s tests
• The documentation builds: nox -s doctests
• Code is commented for hard-to-understand areas
• Tests added that prove fix is effective or that feature works

Additional context

Stop Using NumPy's Global Random Seed – while the article mentions using np.random.default_rng() is a better way, np.random.seed() should be sufficient for now. The rationale is that we don't want to be one of those packages that resets the global random seed set by a user at the top of their script (or by another library that sets it).

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[agriyakhetarpal]

This is a user-facing change as it fixes is/was a slight bug in the solver functionality, but I don't think the fix warrants adding a CHANGELOG entry. What do you think, @kratman?

[kratman]

I have not looked at the code yet, but from your description I don't think it would need one

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.70%. Comparing base (8235474) to head (51bce0f).
Report is 2 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #4847   +/-   ##
========================================
  Coverage    98.70%   98.70%           
========================================
  Files          303      303           
  Lines        23335    23338    +3     
========================================
+ Hits         23032    23035    +3     
  Misses         303      303           

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

[Saransh-cpp]

I remember this. Thanks for the fix, @agriyakhetarpal!

[agriyakhetarpal]

I re-triggered the failing Windows test, as it looks like a flake that has been showing up a bit more often recently:

FAILED tests/unit/test_solvers/test_idaklu_solver.py::TestIDAKLUSolver::test_multiple_inputs - ValueError: Unknown exception

[kratman]

I re-triggered the failing Windows test, as it looks like a flake that has been showing up a bit more often recently:

FAILED tests/unit/test_solvers/test_idaklu_solver.py::TestIDAKLUSolver::test_multiple_inputs - ValueError: Unknown exception

Yeah I have not fully debugged that issue yet. It started appearing after I enabled the IDAKLU tests on windows. The issue was probably there for a while, but not caught. I am hoping I end up tracking it down as I keep working on the PyBaMM solvers build/test issues