Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Deprecate number of electrodes in parallel parameter #4854

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open

Deprecate number of electrodes in parallel parameter #4854

wants to merge 7 commits into from
+27 −55

Conversation

vidipsingh
Copy link
Contributor

Description

This PR deprecates the n_electrodes_parallel parameter due to its inconsistent usage across the codebase. Area calculations in the thermal models have been updated to use the total effective area instead.

Fixes: #4833

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #)

Important checks:

Please confirm the following before marking the PR as ready for review:

  • No style issues: nox -s pre-commit
  • All tests pass: nox -s tests
  • The documentation builds: nox -s doctests
  • Code is commented for hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

vidipsingh and others added 7 commits January 26, 2025 22:22
@vidipsingh
implement hatch-vcs versioning
d79a588
@kratman
Merge branch 'develop' into implement-hatch-vcs-versioning
3ba901f
@vidipsingh
Merge branch 'develop' into implement-hatch-vcs-versioning
28c871d
@vidipsingh
integrate hatch-vcs for automated versioning
6eff861 
- Move hatch-vcs config to `tool.hatch` in `pyproject.toml`
- Switch to `_version.py` for versioning with hatch-vcs
- Remove manual `version.py` updates, using git tags instead
@vidipsingh
Merge branch 'develop' into implement-hatch-vcs-versioning
0f3ae91
@vidipsingh
remove manual version update
f0137f1 
- Removed manual version writing in `update_version.py`.
- Added `src/pybamm/_version.py` to `.gitignore`.
@vidipsingh
Deprecate n_electrodes_parallel parameter
23e4085 
- Deprecate the `n_electrodes_parallel` parameter due to inconsistent usage.
- Updated area calculations in thermal models to use total effective area.
valentinsulzer
valentinsulzer reviewed Feb 15, 2025
View reviewed changes
Copy link
Member

@valentinsulzer valentinsulzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to decide what to do about Ai2020 and Sulzer2019 - perhaps just scaling the electrode width. Also this will need a clear deprecation message (we could check specifically for when people give a value that is not equal to 1 and raise a warning in that case)

src/pybamm/parameters/electrical_parameters.py
@@ -37,7 +37,7 @@ def _set_parameters(self):
"Current function [A]", {"Time [s]": pybamm.t}
)
self.current_density_with_time = self.current_with_time / (
self.n_electrodes_parallel * self.geo.A_cc
self.n_electrodes_pair * self.geo.A_cc
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this kept here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@valentinsulzer should I completely remove this, or is there another way I should handle this? I wasn't sure how to approach this, so I changed the name from n_electrodes_parallel to n_electrodes_pair. Would appreciate your guidance on the right approach here.

@vidipsingh
Copy link
Contributor Author

Need to decide what to do about Ai2020 and Sulzer2019 - perhaps just scaling the electrode width. Also this will need a clear deprecation message (we could check specifically for when people give a value that is not equal to 1 and raise a warning in that case)

@valentinsulzer Thanks for the suggestion! For Ai2020 and Sulzer2019, I'll look into scaling the electrode width as you mentioned.

Regarding the deprecation message, I can add a check for when the n_electrodes_parallel value is not equal to 1 and raise a warning to inform users about the change. Could you please guide me on where exactly these messages should be added in the code?

@DrSOKane
Copy link
Contributor

DrSOKane commented Feb 16, 2025 •
edited
Loading

I see why you want to do this but it can be a bit of a shock to people new to PyBaMM. I was just mentoring someone who was struggling to get the correct capacity and didn't realize that they needed to double the electrode length (compared to what it was in the paper) to account for it being double-sided, which implies the authors of that paper did it a different way than Chen et al.

@aabills
Copy link
Contributor

aabills commented Feb 16, 2025

Thanks for taking this on, @vidipsingh . I can't help but notice that there are a bunch of changes made here that are unrelated to this issue. Perhaps they are related to #4802 ? It would be good to start this from a clean branch to ensure that nothing extraneous is considered here.

@aabills
Copy link
Contributor

aabills commented Feb 16, 2025 •
edited
Loading

I see why you want to do this but it can be a bit of a shock to people new to PyBaMM. I was just mentoring someone who was struggling to get the correct capacity and didn't realize that they needed to double the electrode length (compared to what it was in the paper) to account for it being double-sided, which implies the authors of that paper did it a different way than Chen et al.

My argument is just that we should either make everything consistent with this parameter, or deprecate it, of which I thought deprecation made more sense. Maybe we should debate this at the original issue, #4833

@vidipsingh
Copy link
Contributor Author

Thanks for taking this on, @vidipsingh . I can't help but notice that there are a bunch of changes made here that are unrelated to this issue. Perhaps they are related to #4802 ? It would be good to start this from a clean branch to ensure that nothing extraneous is considered here.

Thanks for pointing that out, @aabills. I did start from a clean branch, so I'm not entirely sure how the changes from that PR ended up here. What would you suggest I do at this point? If I start from a new branch, I'll likely need to close this PR and raise a new one.

@aabills
Copy link
Contributor

aabills commented Feb 17, 2025

Thanks for taking this on, @vidipsingh . I can't help but notice that there are a bunch of changes made here that are unrelated to this issue. Perhaps they are related to #4802 ? It would be good to start this from a clean branch to ensure that nothing extraneous is considered here.

Thanks for pointing that out, @aabills. I did start from a clean branch, so I'm not entirely sure how the changes from that PR ended up here. What would you suggest I do at this point? If I start from a new branch, I'll likely need to close this PR and raise a new one.

Seems reasonable to me, make sure you check out discussion in #4833 first.

@vidipsingh
Copy link
Contributor Author

Thanks for taking this on, @vidipsingh . I can't help but notice that there are a bunch of changes made here that are unrelated to this issue. Perhaps they are related to #4802 ? It would be good to start this from a clean branch to ensure that nothing extraneous is considered here.

Thanks for pointing that out, @aabills. I did start from a clean branch, so I'm not entirely sure how the changes from that PR ended up here. What would you suggest I do at this point? If I start from a new branch, I'll likely need to close this PR and raise a new one.

Seems reasonable to me, make sure you check out discussion in #4833 first.

Sure, will check that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@valentinsulzer valentinsulzer valentinsulzer left review comments

@martinjrobins martinjrobins Awaiting requested review from martinjrobins

@Saransh-cpp Saransh-cpp Awaiting requested review from Saransh-cpp

@agriyakhetarpal agriyakhetarpal Awaiting requested review from agriyakhetarpal

@kratman kratman Awaiting requested review from kratman

@arjxn-py arjxn-py Awaiting requested review from arjxn-py

@brosaplanella brosaplanella Awaiting requested review from brosaplanella

@DrSOKane DrSOKane Awaiting requested review from DrSOKane

@rtimms rtimms Awaiting requested review from rtimms

@TomTranter TomTranter Awaiting requested review from TomTranter

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Deprecate number of electrodes in parallel to make a cell
5 participants
@vidipsingh @DrSOKane @aabills @valentinsulzer @kratman