Implement discharge capacity as an optional x-axis in QuickPlot#4775
Implement discharge capacity as an optional x-axis in QuickPlot#4775medha-14 wants to merge 22 commits intopybamm-team:mainfrom
discharge capacity as an optional x-axis in QuickPlot#4775Conversation
|
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 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
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 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hello @agriyakhetarpal could please give a review on this? :) |
There was a problem hiding this comment.
Thanks, this is a good start.
Variables should always be evaluated with time, not discharge capacity.
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
b0498dc to
5e0b742
Compare
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. |
agriyakhetarpal
left a comment
There was a problem hiding this comment.
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? |
|
For now I have added tests for the function , let me know if any other changes are to be made
Also suggest how to move forward with this? |
valentinsulzer
left a comment
There was a problem hiding this comment.
This is almost there, just a few more comments
|
I have made the suggested changes, if anyone could review this one :) |
|
It looks good to me, just three missing lines in the coverage. Thanks for your work! |
|
I have added tests for the missing lines, are there any other improvements I need to make ? |
agriyakhetarpal
left a comment
There was a problem hiding this comment.
Nice work, thanks, @medha-14! I am happy to approve this, though I'll also request @valentinsulzer for a review as he previously requested changes.
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.
Key checklist:
$ 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)$ python -m pytest(or$ nox -s tests)$ 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: