Issue 4224 duration bug

[valentinsulzer]

Description

Fixes #4224

• longer default duration (1000 hours)
• default duration is 2 / C-rate hours for C-rate steps
• warning if step finishes due to hitting the default duration

Currently working on adding a warning if maximum duration is reached

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

[ejfdickinson]

@valentinsulzer You need to take abs(value) in CRate.default_duration(), because value comes through negative for charge steps.

Alternatively you might think about CRate storing its direction and magnitude in a more descriptive way.

[brosaplanella]

Looks good to me, let's just add a line to CHANGELOG before merging.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.55%. Comparing base (5e04abd) to head (8381ba2).
Report is 216 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4239      +/-   ##
===========================================
+ Coverage    99.54%   99.55%   +0.01%     
===========================================
  Files          288      288              
  Lines        21886    21897      +11     
===========================================
+ Hits         21786    21800      +14     
+ Misses         100       97       -3     

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

[ejfdickinson]

@brosaplanella @valentinsulzer To confirm, this is working as expected for the use cases on which I was testing.

[valentinsulzer]

Changing default duration to 1000 hours caused a severe performance degradation, investigating further

[aabills]

Thanks! I just tried it and though I'd prefer a longer duration barring a substantial performance hit, in my opinion, the bug is fixed by the warning alone.

I wonder if 100 + ε hours might be a happy medium?

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[valentinsulzer]

This PR adds the warning and also fixes the C/100 case which I assume is more common than specifying a small current (backed up by a n=2 sample size from @aabills and @ejfdickinson ). The default duration is 24h and we can reconsider once @MarcBerliner is done with updates to solvers, which will make solve times much less sensitive to things like duration, period, etc