Refactor to match PathSim org conventions#1
Merged
DavidMStraub merged 7 commits intoMar 17, 2026
Conversation
Collaborator
|
Sorry, I missed the PR because notifications were not enabled! Thanks for this! Let me check what the CI failures are. General question: are type hints not allowed in PathSim toolboxes? |
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors PathSim-Batt to align repository structure and style with other PathSim toolbox repos while keeping the public API stable.
Changes:
- Simplifies PyBaMM cell blocks by removing redundant protocol overrides and consolidating lazy initialization logic.
- Updates project metadata and repo hygiene (LICENSE,
.gitignore,project.urls, README format/content). - Adjusts tests to align with the new lazy-init/reset behavior.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/pathsim_batt/cells/pybamm_cell.py |
Refactors PyBaMM-backed cell blocks (lazy init, reset/buffer/step helpers, docstring tweaks). |
src/pathsim_batt/thermal/lumped.py |
Adds PathSim-org style headers and tweaks validation/errors for the lumped thermal block. |
tests/cells/test_pybamm_cell.py |
Updates tests to assert on _sim lazy-init state instead of _initialized. |
pyproject.toml |
Removes Python 3.14 classifier, adds docs optional deps, and adds project.urls. |
README.md |
Streamlines README to org format and adds a PyBaMM integration section. |
LICENSE |
Adds MIT license file. |
.pre-commit-config.yaml |
Removes the pre-commit configuration (ruff hooks). |
.gitignore |
Trims and reorganizes ignore patterns. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
44
to
47
| #parameter setup — mark current and temperature as runtime inputs | ||
| if parameter_values is None: | ||
| parameter_values = pybamm.ParameterValues("Chen2020") | ||
| parameter_values = parameter_values.copy() |
Comment on lines
+52
to
55
| #input validation | ||
| if mass <= 0: | ||
| raise ValueError(f"mass must be positive, got {mass}") | ||
| raise ValueError(f"'mass' must be positive but is {mass}") | ||
| if Cp <= 0: |
Comment on lines
39
to
+44
| ```python | ||
| import pybamm | ||
| from pathsim import Simulation, Connection | ||
| from pathsim.blocks import Constant, Scope | ||
| from pathsim.solvers import SSPRK22 | ||
| from pathsim_batt import CellElectrothermal | ||
|
|
||
| cell = CellElectrothermal(parameter_values=pybamm.ParameterValues("Chen2020")) | ||
| I_app = Constant(5.0) # 1 C | ||
| T_amb = Constant(298.15) | ||
| scope = Scope(labels=["V [V]", "SOC"]) | ||
|
|
||
| Sim = Simulation( | ||
| blocks=[cell, I_app, T_amb, scope], | ||
| connections=[ | ||
| Connection(I_app, cell["I"]), | ||
| Connection(T_amb, cell["T_amb"]), | ||
| Connection(cell["V"], scope[0]), | ||
| Connection(cell["SOC"], scope[1]), | ||
| ], | ||
| dt=10.0, Solver=SSPRK22, | ||
| ) | ||
|
|
||
| Sim.run(3600) | ||
| scope.plot() | ||
| ``` | ||
|
|
||
| ### 2 — External thermal coupling (`CellElectrical` + `LumpedThermal`) | ||
|
|
||
| When you want PathSim to own the thermal ODE — for example to couple multiple cells to a shared cooling model: | ||
|
|
||
| ```python | ||
| import numpy as np, pybamm | ||
| from pathsim import Simulation, Connection | ||
| from pathsim.blocks import Constant, Scope, Amplifier | ||
| from pathsim.solvers import SSPRK22 | ||
| from pathsim_batt import CellElectrical | ||
| from pathsim_batt.thermal import LumpedThermal | ||
|
|
||
| cell_volume = np.pi * 0.0105**2 * 0.070 # m³ (LG M50 21700) | ||
|
|
||
| cell = CellElectrical(parameter_values=pybamm.ParameterValues("Chen2020")) | ||
| thermal = LumpedThermal(mass=0.065, Cp=750.0, UA=0.5, T0=298.15) | ||
| gain = Amplifier(cell_volume) # W/m³ → W | ||
| I_app = Constant(5.0) | ||
| T_amb = Constant(298.15) | ||
| scope = Scope(labels=["V [V]", "T [K]", "SOC"]) | ||
|
|
||
| Sim = Simulation( | ||
| blocks=[cell, thermal, gain, I_app, T_amb, scope], | ||
| connections=[ | ||
| Connection(I_app, cell["I"]), | ||
| Connection(thermal["T"], cell["T_cell"]), # feedback | ||
| Connection(cell["Q_heat"], gain), | ||
| Connection(gain, thermal["Q_dot"]), | ||
| Connection(T_amb, thermal["T_amb"]), | ||
| Connection(cell["V"], scope[0]), | ||
| Connection(thermal["T"], scope[1]), | ||
| Connection(cell["SOC"], scope[2]), | ||
| ], | ||
| dt=10.0, Solver=SSPRK22, | ||
| ) | ||
|
|
||
| Sim.run(3600) | ||
| scope.plot() | ||
| model = pybamm.lithium_ion.DFN(options={"thermal": "lumped"}) | ||
| params = pybamm.ParameterValues("Mohtat2020") | ||
| cell = CellElectrothermal(model=model, parameter_values=params) |
|
|
||
| # IMPORTS =============================================================================== | ||
|
|
||
| import numpy as np |
Comment on lines
+92
to
98
| def _pybamm_inputs(self): | ||
| """Build PyBaMM input dict from current block inputs.""" | ||
| T = float(self.inputs[1]) or 298.15 | ||
| return { | ||
| "Current function [A]": float(self.inputs[0]), | ||
| "Ambient temperature [K]": T, | ||
| } |
Comment on lines
+60
to
63
| #store parameters | ||
| self.mass = float(mass) | ||
| self.Cp = float(Cp) | ||
| self.UA = float(UA) |
Comment on lines
+52
to
+55
| #solver | ||
| self._pybamm_solver = solver or pybamm.CasadiSolver(mode="safe") | ||
|
|
||
| self._sim: pybamm.Simulation | None = None | ||
| self._initialized = False | ||
| self._sim = None |
| if not self._initialized: | ||
| self._initialize() | ||
| def step(self, t, dt): | ||
| #lazy initialisation on first step |
Collaborator
|
I think the copilot comments are all legit. Let me try to tell it to fix them (not sure this works). |
Collaborator
|
@copilot open a new pull request to apply changes based on the comments in this thread |
Member
Author
|
Type hints are generally allowed but not enforced. Mainly because there is quite a lot of flexibility in types that are allowd for many of the pathsim blocks. |
Member
Author
|
The copilot suggestions make sense |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Collaborator
|
I'll just merge as-is and fix the issues in a new PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Aligns the repo structure and code style with the other PathSim toolbox repos (pathsim-rf, pathsim-chem, pathsim-flight).
.gitignore, addproject.urlstopyproject.tomlPublic API unchanged.