Enhance plot_voltage_components.py for Simulation and Solution classes #3723

AlessioBugetti · 2024-01-15T15:30:07Z

Description

This commit extends the functionality of the plot_voltage_components method, enhancing its integration within the Simulation and Solution classes.

Key Changes:

Modified the plot_voltage_components function in plot_voltage_components.py to handle both Simulation and Solution objects.
Simulation Class: Added a new method plot_voltage_components. This method calls the standalone plot_voltage_components function and utilizes the solution attribute of the Simulation instance to generate the plot.
Solution Class: Introduced a similar plot_voltage_components method to the Solution class, enabling direct plotting of voltage components from Solution instances.
Modified test_plot_voltage_components.py

Fixes #3722

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

This commit extends the functionality of the `plot_voltage_components` method in PyBaMM, enhancing its integration within the `Simulation` and `Solution` classes. Key Changes: - Modified the standalone `plot_voltage_components` function in `plot_voltage_components.py` to handle both `Simulation` and `Solution` objects. - `Simulation` Class: Added a new method `plot_voltage_components`. This method calls the standalone `plot_voltage_components` function and utilizes the solution attribute of the `Simulation` instance to generate the plot. - `Solution` Class: Introduced a similar `plot_voltage_components` method to the `Solution` class, enabling direct plotting of voltage components from `Solution` instances. - Modified `test_plot_voltage_components.py`

Saransh-cpp

Thanks for working on this, @AlessioBugetti!

It would be nice to comb through the example notebooks once and see if the new sim.plot_voltage_components and sol.plot_voltage_components can be added somewhere.

Saransh-cpp · 2024-01-15T15:45:19Z

pybamm/plotting/plot_voltage_components.py

+    else:
+        raise ValueError("Input must be a Solution or Simulation object")


IMO specifying the type of the variable in the docs (docstring) should be enough.

Suggested change

else:

raise ValueError("Input must be a Solution or Simulation object")

review-notebook-app · 2024-01-15T16:52:42Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

brosaplanella · 2024-01-15T18:17:56Z

It would be nice to comb through the example notebooks once and see if the new sim.plot_voltage_components and sol.plot_voltage_components can be added somewhere.

I am reviewing the Getting Started notebooks so will include these in Tutorial 3 once they are merged.

brosaplanella

Looks good, thanks! Just add a line to the CHANGELOG before we merge.

…-voltage-components

codecov · 2024-01-15T20:50:12Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5900ada) 99.59% compared to head (d8d8414) 99.59%.
Report is 2 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3723   +/-   ##
========================================
  Coverage    99.59%   99.59%           
========================================
  Files          258      258           
  Lines        20811    20823   +12     
========================================
+ Hits         20726    20738   +12     
  Misses          85       85

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

…is called on an unsolved simulation

Saransh-cpp · 2024-01-15T22:32:10Z

@all-contributors please add @AlessioBugetti for code, docs, tests

allcontributors · 2024-01-15T22:32:19Z

@Saransh-cpp

I've put up a pull request to add @AlessioBugetti! 🎉

brosaplanella · 2024-01-16T08:32:10Z

CHANGELOG.md

 - Fixed a bug where if the first step(s) in a cycle are skipped then the cycle solution started from the model's initial conditions instead of from the last state of the previous cycle ([#3708](https://github.com/pybamm-team/PyBaMM/pull/3708))
+- Updated `plot_voltage_components.py` to support both `Simulation` and `Solution` objects. Added new methods in both `Simulation` and `Solution` classes for allow the syntax `simulation.plot_voltage_components` and `solution.plot_voltage_components`. Updated `test_plot_voltage_components.py` to reflect these changes ([#3723](https://github.com/pybamm-team/PyBaMM/pull/3723)).


Suggested change

- Fixed a bug where if the first step(s) in a cycle are skipped then the cycle solution started from the model's initial conditions instead of from the last state of the previous cycle ([#3708](https://github.com/pybamm-team/PyBaMM/pull/3708))

- Updated `plot_voltage_components.py` to support both `Simulation` and `Solution` objects. Added new methods in both `Simulation` and `Solution` classes for allow the syntax `simulation.plot_voltage_components` and `solution.plot_voltage_components`. Updated `test_plot_voltage_components.py` to reflect these changes ([#3723](https://github.com/pybamm-team/PyBaMM/pull/3723)).

- Updated `plot_voltage_components.py` to support both `Simulation` and `Solution` objects. Added new methods in both `Simulation` and `Solution` classes for allow the syntax `simulation.plot_voltage_components` and `solution.plot_voltage_components`. Updated `test_plot_voltage_components.py` to reflect these changes ([#3723](https://github.com/pybamm-team/PyBaMM/pull/3723)).

- Fixed a bug where if the first step(s) in a cycle are skipped then the cycle solution started from the model's initial conditions instead of from the last state of the previous cycle ([#3708](https://github.com/pybamm-team/PyBaMM/pull/3708))

Small comment: for the CHANGELOG we sort contributions by decreasing PR number (i.e. newer PRs first).

Sorry, I just corrected this oversight

Commit here

@AlessioBugetti Just as an FYI, you can accept suggestions too so you don't have to do a separate commit. You just need to press the "commit suggestion" button

Saransh-cpp · 2024-01-17T13:36:30Z

The CHANGELOG in develop was updated for v24.1rc1, so you might have to fix that (sorry for the conflicts).

AlessioBugetti · 2024-01-17T13:47:14Z

The CHANGELOG in develop was updated for v24.1rc1, so you might have to fix that (sorry for the conflicts).

I've updated the CHANGELOG, conflicts resolved

Saransh-cpp approved these changes Jan 15, 2024

View reviewed changes

Saransh-cpp requested a review from brosaplanella January 15, 2024 15:54

Saransh-cpp mentioned this pull request Jan 15, 2024

Check if the solution exists before creating a GIF (Simulation.create_gif and BatchStudy.create_gif) #3725

Closed

AlessioBugetti added 2 commits January 15, 2024 17:31

Improved code readability by removing explicit variable type checking

aebcef7

Update example notebooks

fafe800

brosaplanella approved these changes Jan 15, 2024

View reviewed changes

AlessioBugetti added 2 commits January 15, 2024 20:55

Merge remote-tracking branch 'upstream/develop' into improvement-plot…

602f2c3

…-voltage-components

Update CHANGELOG.md

7f578f1

AlessioBugetti added 2 commits January 15, 2024 21:57

Fix test_plot_voltage_components.py

a309b1b

Add test to ensure ValueError is raised when plot_voltage_components …

7ef99a0

…is called on an unsolved simulation

allcontributors bot mentioned this pull request Jan 15, 2024

docs: add AlessioBugetti as a contributor for code, doc, and test #3730

Merged

brosaplanella reviewed Jan 16, 2024

View reviewed changes

Sort contributions by decreasing PR number

86cddb6

Merge branch 'develop' into improvement-plot-voltage-components

d8d8414

Saransh-cpp merged commit 73b470f into pybamm-team:develop Jan 18, 2024
39 of 41 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance plot_voltage_components.py for Simulation and Solution classes #3723

Enhance plot_voltage_components.py for Simulation and Solution classes #3723

AlessioBugetti commented Jan 15, 2024

Saransh-cpp left a comment •

edited

Saransh-cpp Jan 15, 2024

review-notebook-app bot commented Jan 15, 2024

brosaplanella commented Jan 15, 2024

brosaplanella left a comment

codecov bot commented Jan 15, 2024 •

edited

Saransh-cpp commented Jan 15, 2024

allcontributors bot commented Jan 15, 2024

brosaplanella Jan 16, 2024

AlessioBugetti Jan 16, 2024

kratman Jan 16, 2024

Saransh-cpp commented Jan 17, 2024

AlessioBugetti commented Jan 17, 2024

		else:
		raise ValueError("Input must be a Solution or Simulation object")

		- Fixed a bug where if the first step(s) in a cycle are skipped then the cycle solution started from the model's initial conditions instead of from the last state of the previous cycle ([#3708](https://github.com/pybamm-team/PyBaMM/pull/3708))
		- Updated `plot_voltage_components.py` to support both `Simulation` and `Solution` objects. Added new methods in both `Simulation` and `Solution` classes for allow the syntax `simulation.plot_voltage_components` and `solution.plot_voltage_components`. Updated `test_plot_voltage_components.py` to reflect these changes ([#3723](https://github.com/pybamm-team/PyBaMM/pull/3723)).

Enhance plot_voltage_components.py for Simulation and Solution classes #3723

Enhance plot_voltage_components.py for Simulation and Solution classes #3723

Conversation

AlessioBugetti commented Jan 15, 2024

Description

Type of change

Key checklist:

Further checks:

Saransh-cpp left a comment • edited

Choose a reason for hiding this comment

Saransh-cpp Jan 15, 2024

Choose a reason for hiding this comment

review-notebook-app bot commented Jan 15, 2024

brosaplanella commented Jan 15, 2024

brosaplanella left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 15, 2024 • edited

Codecov Report

Saransh-cpp commented Jan 15, 2024

allcontributors bot commented Jan 15, 2024

brosaplanella Jan 16, 2024

Choose a reason for hiding this comment

AlessioBugetti Jan 16, 2024

Choose a reason for hiding this comment

kratman Jan 16, 2024

Choose a reason for hiding this comment

Saransh-cpp commented Jan 17, 2024

AlessioBugetti commented Jan 17, 2024

Saransh-cpp left a comment •

edited

codecov bot commented Jan 15, 2024 •

edited