PyBaMM icon indicating copy to clipboard operation
PyBaMM copied to clipboard

Extend Step String Tagging Functionality

Open mleot opened this issue 1 year ago • 5 comments

Description

Adds additional funcitonality around step string tags. Allows for filtering a solution object based on tags. Allows for inclusion of tags when exporting data through the Solution.get_data_dict() method.

Fixes #4177

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.

  • [x] 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

mleot avatar Jun 14 '24 01:06 mleot

Codecov Report

Attention: Patch coverage is 89.47368% with 2 lines in your changes missing coverage. Please review.

Project coverage is 99.54%. Comparing base (205ca81) to head (35e72a9). Report is 3 commits behind head on develop.

:exclamation: Current head 35e72a9 differs from pull request most recent head 7916a2b

Please upload reports for the commit 7916a2b to get more accurate results.

Files Patch % Lines
pybamm/solvers/solution.py 88.23% 2 Missing :warning:
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4178      +/-   ##
===========================================
- Coverage    99.55%   99.54%   -0.01%     
===========================================
  Files          288      288              
  Lines        21857    21876      +19     
===========================================
+ Hits         21760    21777      +17     
- Misses          97       99       +2     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Jun 14 '24 18:06 codecov[bot]

@valentinsulzer I added an additional feature, being the .step and .cycle attributes to the solution objects, which are assigned during the solve step of a simulation, when running 'with experiment'.

mleot avatar Jun 24 '24 18:06 mleot

Are assert statements generally frowned upon in testing? I noticed that it raises issues in the static code analysis.

mleot avatar Jun 25 '24 16:06 mleot

Are assert statements generally frowned upon in testing? I noticed that it raises issues in the static code analysis.

They are usually frowned upon in regular code, but I think we need to make Codacy more lenient about this a bit, because these are all in tests/, where asserts are commonplace. Soon, the use of asserts will grow a lot in PyBaMM (see #4156). I am unsure who has access to the relevant settings and if we can configure it with a codacy.yml file of sorts.

agriyakhetarpal avatar Jun 25 '24 20:06 agriyakhetarpal

@mleot are you still working on this?

kratman avatar Nov 04 '24 22:11 kratman