Modifies `QuickPlot.plot` to take a list of times and show superimposed plots in case of 1D variables. #4529

medha-14 · 2024-10-20T14:10:30Z

Description

Fixes #4271
Related to #3164

model = pybamm.lithium_ion.DFN()
sim = pybamm.Simulation(model)
sim.solve([0, 3600])
plot = QuickPlot(sim)
plot.plot([0.5,0.6,0.8])

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

medha-14 · 2024-10-20T14:13:17Z

I will be working on adding different colors and a legend to the time plots to make them easier to differentiate. Please let me know if you have any suggestions for changes or improvements.

medha-14 · 2024-10-21T13:23:59Z

I've added different colors to the time plots to make them easier to differentiate. Additionally, I've fixed some of the failing tests. Please suggest any changes or improvements.

arjxn-py

Awesome work @medha-14, you might just want to look into the keyerror that's being raised.
You can try debugging that by running one test at a time something like - pytest tests/unit/test_plotting/test_quick_plot.py::test_plot_1plus1D_spme Otherwise this looks really good.

src/pybamm/plotting/quick_plot.py

tests/unit/test_plotting/test_quick_plot.py

medha-14 · 2024-10-23T12:36:18Z

I will work on fixing the failing tests. Should I also start working on adding new tests for this method?

kratman · 2024-10-23T13:26:09Z

I will work on fixing the failing tests. Should I also start working on adding new tests for this method?

@medha-14 Coverage reports will only start appearing for this PR after tests pass. You will need to make sure all new code is covered by tests as well as fixing the existing tests

medha-14 · 2024-10-27T10:53:42Z

I have fixed the failing tests and tried to add tests , using a list of time in the plot function. I am open to work on any suggested improvements.

medha-14 · 2024-11-02T10:26:34Z

Hi @kratman, could you review this and let me know what further changes I should make?

codecov · 2024-11-03T11:59:11Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.22%. Comparing base (204c076) to head (4bea86c).
Report is 1 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4529      +/-   ##
===========================================
- Coverage    99.25%   99.22%   -0.04%     
===========================================
  Files          302      302              
  Lines        22838    22821      -17     
===========================================
- Hits         22667    22643      -24     
- Misses         171      178       +7

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

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

medha-14 · 2024-11-11T11:28:48Z

Are there any other changes or improvements that I could make to further enhance the implementation?

kratman · 2024-11-21T02:13:20Z

@medha-14 Sorry for the delay on this one, I will start reviewing this again

kratman

Small findings, I will do a full review another day

src/pybamm/plotting/quick_plot.py

kratman · 2024-11-21T02:22:16Z

Please also add a change log entry. Note that we are doing a release, so once #4598 is merged, there will likely be a merge conflict in the changelog

kratman

This should be the last round of findings. Once these are resolved I will approve. Please double check that the figures still appear as you originally intended. Just good to do a visual double check. I will review those as well

CHANGELOG.md

src/pybamm/plotting/quick_plot.py

agriyakhetarpal

Thanks, @medha-14! I only skimmed through the changes and haven't been involved in the review here, I'll request a review from Eric for the final merge.

CHANGELOG.md

Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>

kratman

Thanks looks good

kratman · 2024-11-22T15:55:11Z

@allcontributors add @medha-14 for code

allcontributors · 2024-11-22T15:55:15Z

@kratman

@medha-14 already contributed before to code

kratman · 2024-11-22T16:00:24Z

I will merge once I see the windows 3.9 check pass. I do not expect a problem, so this should be merged in a couple minutes

medha-14 · 2024-11-22T16:06:50Z

Thanks a lot everyone!

…ed plots in case of 1D variables. (pybamm-team#4529) * Modified plot method to take a list of times . * fixed failing tests * added different colours to different time plots * fixed failing tests * added tests for using list of times * added changelog enrty * made small changes * Update CHANGELOG.md Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com> --------- Co-authored-by: Arjun Verma <arjunverma.oc@gmail.com> Co-authored-by: Eric G. Kratz <kratman@users.noreply.github.com> Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>

…perimposed plots in case of 1D variables. (pybamm-team#4529)" This reverts commit b50d4a9.

* Revert "Modifies `QuickPlot.plot` to take a list of times and show superimposed plots in case of 1D variables. (#4529)" This reverts commit b50d4a9. * style: pre-commit fixes * Fix changelog with PR number --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

Modified plot method to take a list of times .

de4f967

medha-14 requested review from martinjrobins, rtimms, Saransh-cpp, valentinsulzer, kratman and agriyakhetarpal as code owners October 20, 2024 14:10

medha-14 added 2 commits October 20, 2024 22:08

fixed failing tests

8f52938

added different colours to different time plots

173d514

Merge branch 'develop' into quickplot_plot

bc1b47a

arjxn-py reviewed Oct 22, 2024

View reviewed changes

This comment was marked as duplicate.

Sign in to view

kratman requested changes Oct 22, 2024

View reviewed changes

src/pybamm/plotting/quick_plot.py Outdated Show resolved Hide resolved

src/pybamm/plotting/quick_plot.py Outdated Show resolved Hide resolved

tests/unit/test_plotting/test_quick_plot.py Outdated Show resolved Hide resolved

medha-14 added 2 commits October 27, 2024 01:05

fixed failing tests

8313c81

added tests for using list of times

650b156

Merge branch 'develop' into quickplot_plot

69d730f

medha-14 requested review from kratman and arjxn-py October 28, 2024 09:02

kratman and others added 2 commits November 4, 2024 09:13

Merge branch 'develop' into quickplot_plot

ba9ecbe

Merge branch 'develop' into quickplot_plot

7a637ce

medha-14 and others added 2 commits November 14, 2024 07:25

Merge branch 'develop' into quickplot_plot

af50e15

Merge branch 'develop' into quickplot_plot

678d82e

kratman reviewed Nov 21, 2024

View reviewed changes

src/pybamm/plotting/quick_plot.py Outdated Show resolved Hide resolved

src/pybamm/plotting/quick_plot.py Outdated Show resolved Hide resolved

src/pybamm/plotting/quick_plot.py Outdated Show resolved Hide resolved

medha-14 and others added 2 commits November 21, 2024 10:34

added changelog enrty

e8b9d26

Merge branch 'develop' into quickplot_plot

8443c92

kratman requested changes Nov 21, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

src/pybamm/plotting/quick_plot.py Outdated Show resolved Hide resolved

src/pybamm/plotting/quick_plot.py Outdated Show resolved Hide resolved

src/pybamm/plotting/quick_plot.py Show resolved Hide resolved

made small changes

e9e0963

agriyakhetarpal previously approved these changes Nov 22, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

agriyakhetarpal requested a review from kratman November 22, 2024 15:17

Update CHANGELOG.md

4bea86c

Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>

kratman dismissed agriyakhetarpal’s stale review via 4bea86c November 22, 2024 15:32

kratman approved these changes Nov 22, 2024

View reviewed changes

kratman merged commit eefa374 into pybamm-team:develop Nov 22, 2024
25 of 26 checks passed

medha-14 deleted the quickplot_plot branch November 22, 2024 16:15

rtimms mentioned this pull request Nov 26, 2024

[Bug]: Variables names missing from plot #4620

Closed

kratman added a commit to kratman/PyBaMM that referenced this pull request Nov 27, 2024

Revert "Modifies QuickPlot.plot to take a list of times and show su…

ca4aeba

…perimposed plots in case of 1D variables. (pybamm-team#4529)" This reverts commit b50d4a9.

kratman mentioned this pull request Nov 27, 2024

Release v24.11.2 #4624

Merged

agriyakhetarpal mentioned this pull request Nov 28, 2024

pybamm v24.11.2 conda-forge/pybamm-feedstock#69

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modifies `QuickPlot.plot` to take a list of times and show superimposed plots in case of 1D variables. #4529

Modifies `QuickPlot.plot` to take a list of times and show superimposed plots in case of 1D variables. #4529

medha-14 commented Oct 20, 2024 •

edited

Loading

medha-14 commented Oct 20, 2024

medha-14 commented Oct 21, 2024

arjxn-py left a comment

This comment was marked as duplicate.

medha-14 commented Oct 23, 2024

kratman commented Oct 23, 2024

medha-14 commented Oct 27, 2024

medha-14 commented Nov 2, 2024

codecov bot commented Nov 3, 2024 •

edited

Loading

medha-14 commented Nov 11, 2024

kratman commented Nov 21, 2024

kratman left a comment

kratman commented Nov 21, 2024

kratman left a comment

agriyakhetarpal left a comment

kratman left a comment

kratman commented Nov 22, 2024

allcontributors bot commented Nov 22, 2024

kratman commented Nov 22, 2024

medha-14 commented Nov 22, 2024

Modifies QuickPlot.plot to take a list of times and show superimposed plots in case of 1D variables. #4529

Modifies QuickPlot.plot to take a list of times and show superimposed plots in case of 1D variables. #4529

Conversation

medha-14 commented Oct 20, 2024 • edited Loading

Description

Type of change

Key checklist:

Further checks:

medha-14 commented Oct 20, 2024

medha-14 commented Oct 21, 2024

arjxn-py left a comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

medha-14 commented Oct 23, 2024

kratman commented Oct 23, 2024

medha-14 commented Oct 27, 2024

medha-14 commented Nov 2, 2024

codecov bot commented Nov 3, 2024 • edited Loading

Codecov Report

medha-14 commented Nov 11, 2024

kratman commented Nov 21, 2024

kratman left a comment

Choose a reason for hiding this comment

kratman commented Nov 21, 2024

kratman left a comment

Choose a reason for hiding this comment

agriyakhetarpal left a comment

Choose a reason for hiding this comment

kratman left a comment

Choose a reason for hiding this comment

kratman commented Nov 22, 2024

allcontributors bot commented Nov 22, 2024

kratman commented Nov 22, 2024

medha-14 commented Nov 22, 2024

Modifies `QuickPlot.plot` to take a list of times and show superimposed plots in case of 1D variables. #4529

Modifies `QuickPlot.plot` to take a list of times and show superimposed plots in case of 1D variables. #4529

medha-14 commented Oct 20, 2024 •

edited

Loading

codecov bot commented Nov 3, 2024 •

edited

Loading