PyBaMM icon indicating copy to clipboard operation
PyBaMM copied to clipboard

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

Open medha-14 opened this issue 1 year ago • 4 comments

Description

image

Fixes #4271

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:

  • [x] 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 avatar Oct 20 '24 14:10 medha-14

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 avatar Oct 20 '24 14:10 medha-14

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.

image

medha-14 avatar Oct 21 '24 13:10 medha-14

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

medha-14 avatar Oct 23 '24 12:10 medha-14

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

kratman avatar Oct 23 '24 13:10 kratman

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 avatar Oct 27 '24 10:10 medha-14

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

medha-14 avatar Nov 02 '24 10:11 medha-14

Codecov Report

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

Project coverage is 99.22%. Comparing base (204c076) to head (4bea86c). Report is 166 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     

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

codecov[bot] avatar Nov 03 '24 11:11 codecov[bot]

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

medha-14 avatar Nov 11 '24 11:11 medha-14

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

kratman avatar Nov 21 '24 02:11 kratman

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 avatar Nov 21 '24 02:11 kratman

@allcontributors add @medha-14 for code

kratman avatar Nov 22 '24 15:11 kratman

@kratman

@medha-14 already contributed before to code

allcontributors[bot] avatar Nov 22 '24 15:11 allcontributors[bot]

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

kratman avatar Nov 22 '24 16:11 kratman

Thanks a lot everyone!

medha-14 avatar Nov 22 '24 16:11 medha-14