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

Refactor get_initial_stoichiometries with custom and default tolerance #3714

Merged
merged 10 commits into from
Jan 24, 2024

Conversation

AlessioBugetti
Copy link
Contributor

@AlessioBugetti AlessioBugetti commented Jan 12, 2024

Description

This commit enhances the get_initial_stoichiometries method by introducing a new parameter, solver_tolerance. The primary change allows users to specify a custom tolerance level for the solver.

The method now includes a default solver tolerance set at 1e-6, maintaining a balance between precision and computational efficiency for general use cases. Additionally, users can adjust this tolerance level as needed, catering to specific requirements of different battery modeling scenarios.

Fixes #3688

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

AlessioBugetti and others added 2 commits January 12, 2024 02:07
This commit enhances the `get_initial_stoichiometries` method by introducing a new parameter, `solver_tolerance`. The primary change allows users to specify a custom tolerance level for the solver.

The method now includes a default solver tolerance set at 1e-6, maintaining a balance between precision and computational efficiency for general use cases. Additionally, users can adjust this tolerance level as needed, catering to specific requirements of different battery modeling scenarios.

Resolves: #3688
Copy link

codecov bot commented Jan 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2f4a928) 99.59% compared to head (96dbb38) 99.59%.
Report is 2 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3714   +/-   ##
========================================
  Coverage    99.59%   99.59%           
========================================
  Files          258      258           
  Lines        20823    20825    +2     
========================================
+ Hits         20738    20740    +2     
  Misses          85       85           

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

@agriyakhetarpal
Copy link
Member

Question: should we set the function argument to be just tol, instead of solver_tolerance – to be in line with that for the AlgebraicSolver class?

@AlessioBugetti
Copy link
Contributor Author

I've updated the function argument name from solver_tolerance to tol in the latest commit. This change brings consistency with the naming convention used in the AlgebraicSolver class.

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.

This seems fine to me as long as the tests pass

@AlessioBugetti
Copy link
Contributor Author

AlessioBugetti commented Jan 24, 2024

I see that the Run benchmarks on push / benchmarks fail, let me know if any changes are required or if the PR can be merged

@agriyakhetarpal
Copy link
Member

I see that the Run benchmarks on push / benchmarks fail, let me know if any changes are required or if the PR can be merged

Not to worry, the benchmarks on PRs have always been flaky, and I don't think the changes here can influence them in any way. Let's wait till @rtimms's review as the author of the linked issue, and it should be good to merge after that

@kratman
Copy link
Contributor

kratman commented Jan 24, 2024

@AlessioBugetti Can you just add a small summary to the change log?

@AlessioBugetti
Copy link
Contributor Author

@AlessioBugetti Can you just add a small summary to the change log?

Yes, of course. I have updated the change log as requested.

@kratman
Copy link
Contributor

kratman commented Jan 24, 2024

@AlessioBugetti Sorry to do this, but I think it should be in the unreleased section not the RC section for the change log

@AlessioBugetti
Copy link
Contributor Author

@AlessioBugetti Sorry to do this, but I think it should be in the unreleased section not the RC section for the change log

Yes, I have made the corrections as requested

@kratman
Copy link
Contributor

kratman commented Jan 24, 2024

Thanks, just waiting for the tests to pass now

@kratman kratman merged commit 0ac01eb into pybamm-team:develop Jan 24, 2024
40 of 41 checks passed
js1tr3 pushed a commit to js1tr3/PyBaMM that referenced this pull request Aug 12, 2024
pybamm-team#3714)

* Refactor get_initial_stoichiometries with custom and default tolerance

This commit enhances the `get_initial_stoichiometries` method by introducing a new parameter, `solver_tolerance`. The primary change allows users to specify a custom tolerance level for the solver.

The method now includes a default solver tolerance set at 1e-6, maintaining a balance between precision and computational efficiency for general use cases. Additionally, users can adjust this tolerance level as needed, catering to specific requirements of different battery modeling scenarios.

Resolves: pybamm-team#3688

* style: pre-commit fixes

* Refactor argument name from 'solver_tolerance' to 'tol' for consistency with AlgebraicSolver class

* Update CHANGELOG.md

* Fix CHANGELOG.md

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Eric G. Kratz <kratman@users.noreply.github.com>
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.

Set solver tolerances in ElectrodeSOH classes and methods
3 participants