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

Realign 'count' increment #2986

Merged
merged 5 commits into from
May 30, 2023
Merged

Realign 'count' increment #2986

merged 5 commits into from
May 30, 2023

Conversation

julian-evers
Copy link
Contributor

@julian-evers julian-evers commented May 24, 2023

Description

Increment 'count' adjacent to the 'dt' change to avoid an off-by-one error of 'max_count'.

Fixes #2964

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)
  • [ x] Bug fix (non-breaking change which fixes an issue)

Key checklist:

  • [ x] 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)
  • [ x] All tests pass: $ python run-tests.py --all
  • [x ] 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 May 24, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (9d60f05) 99.71% compared to head (4645b66) 99.71%.

Additional details and impacted files
@@            Coverage Diff            @@
##           develop    #2986    +/-   ##
=========================================
  Coverage    99.71%   99.71%            
=========================================
  Files          273      245    -28     
  Lines        19032    18705   -327     
=========================================
- Hits         18977    18651   -326     
+ Misses          55       54     -1     
Impacted Files Coverage Δ
pybamm/solvers/casadi_solver.py 99.66% <100.00%> (ø)

... and 33 files with indirect coverage changes

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

Copy link
Contributor

@rtimms rtimms left a comment

Choose a reason for hiding this comment

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

thanks, looks good! the failing notebook (examples/notebooks/parameterization/parameter-values.ipynb) looks unrelated to your changes - does it run locally for you?

# also reduce maximum step size for future global steps,
# but skip them in the beginning
# sometimes, for the first integrator smaller timesteps are
# needed, but this won't affect the global timesteps. The
# global timestep will only be reduced after the first timestep.
if first_ts_solved:
dt_max = dt
if count >= self.max_step_decrease_count:
if count - 1 >= self.max_step_decrease_count:
Copy link
Contributor

Choose a reason for hiding this comment

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

would just doing count > self.max_step_decrease_count be simpler to read?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just doing count > self.max_step_decrease_count would be simpler to read, I have changed it.
The "parameter-values" notebook runs locally for me, but I noticed that there was a different value assigned for a parameter (b) in the markdown cell than in the code cell of the "Solving a model" section and assigned it to the value of the code cell.

@brosaplanella
Copy link
Member

brosaplanella commented May 25, 2023

thanks, looks good! the failing notebook (examples/notebooks/parameterization/parameter-values.ipynb) looks unrelated to your changes - does it run locally for you?

The example fails because in one of the lines it uses a parameter_values dict that does not have all the required values (unrelated to the changes here). @julian-evers , it should be an easy fix if you want to address that in this PR.

@rtimms
Copy link
Contributor

rtimms commented May 26, 2023

The failing notebook is unrelated to this PR and will be fixed as part of #2957

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link
Contributor

@rtimms rtimms left a comment

Choose a reason for hiding this comment

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

thanks @julian-evers looks good!

@rtimms
Copy link
Contributor

rtimms commented May 30, 2023

@all-contributors please add @julian-evers for code

@rtimms rtimms merged commit 56b63e9 into pybamm-team:develop May 30, 2023
@allcontributors
Copy link
Contributor

@rtimms

I've put up a pull request to add @julian-evers! 🎉

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]: CasadiSolver._integrate(): is increment of "count" misaligned?
3 participants