PyBaMM icon indicating copy to clipboard operation
PyBaMM copied to clipboard

Implement `discharge capacity` as an optional x-axis in `QuickPlot`

Open medha-14 opened this issue 11 months ago • 9 comments

Description

Fixes #1751

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 -m pytest (or $ nox -s tests)
  • [ ] The documentation builds: $ python -m pytest --doctest-plus src (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ 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

medha-14 avatar Jan 19 '25 05:01 medha-14

I have implemented the necessary changes for handling 0D variables. Once this is confirmed to be working correctly, I plan to work on handling 1D and 2D variables. Additionally, I have made changes in the dynamic plot, though I am unsure if they were necessary, so I would appreciate it if you could review them as well. Could you please suggest the next steps for moving forward?

medha-14 avatar Jan 19 '25 05:01 medha-14

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 98.58%. Comparing base (72df274) to head (cfcd43f).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #4775   +/-   ##
========================================
  Coverage    98.57%   98.58%           
========================================
  Files          304      304           
  Lines        23656    23675   +19     
========================================
+ Hits         23320    23339   +19     
  Misses         336      336           

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

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Jan 19 '25 06:01 codecov[bot]

Hello @agriyakhetarpal could please give a review on this? :)

medha-14 avatar Jan 22 '25 13:01 medha-14

Also, the implementation would be simpler if you just set self.x_axis, self.x_min and self.x_max once and used those everywhere, instead of having the if/else for time and discharge capacity in so many places

I have made the suggested changes, I would like a review on if the plot functionality is working correctly or do I have to make some other changes.

medha-14 avatar Feb 05 '25 09:02 medha-14

I think the change looks good, could you please add a test or two? I expect the coverage to go down as you have a few branches in the code now (i.e., one for time, and one for discharge capacity).

Do I also have to make similar changes for 1D and 2D variables? or are they simply not applicable in those cases?

medha-14 avatar Feb 10 '25 09:02 medha-14

For now I have added tests for the function , let me know if any other changes are to be made

Do I also have to make similar changes for 1D and 2D variables? or are they simply not applicable in those cases?

Also suggest how to move forward with this?

medha-14 avatar Feb 11 '25 04:02 medha-14

I have made the suggested changes, if anyone could review this one :)

medha-14 avatar Feb 15 '25 20:02 medha-14

It looks good to me, just three missing lines in the coverage. Thanks for your work!

agriyakhetarpal avatar Feb 16 '25 04:02 agriyakhetarpal

I have added tests for the missing lines, are there any other improvements I need to make ?

medha-14 avatar Feb 19 '25 14:02 medha-14