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.

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

Removed sd_a- particle size parameters: mean, standard deviation #3226

Conversation

jeromtom
Copy link
Contributor

@jeromtom jeromtom commented Aug 2, 2023

Description

Problem:
Particle size parameters like mean/standard deviation are defined but not used, potentially leading to a situation where the user changes it but it will have no effect

Considerations:
-There isn't one mean standard deviation parameter in size_distribution_parameter.py but sd_p and sd_n for the area-weighted standard deviation of positive and negative electrodes respectively.
-sd_p and sd_n cannot be directly removed as they are used in the calculations of R_min and R_max which like you said determine the geometry. We can remove to parameter by assigning the default value of 0.3 in the calculations.

Solution:
Removed sd_a parameter (area-weighted particle-size standard deviation) from the codebase.

Fixes #3219

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Key checklist:

  • No style issues: $ pre-commit run (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
  • The documentation builds: $ python run-tests.py --doctest

You can run unit and doctests together at once, using $ python run-tests.py --quick.

@jeromtom
Copy link
Contributor Author

jeromtom commented Aug 2, 2023

image
I think the docs build failed because I used a special character in the pull request head to highlight that sd_a is a parameter. The local docs build test had passed.

@jeromtom jeromtom changed the title Removed sd_a- particle size parameters: mean, standard deviation Removed sd_a- particle size parameters: mean, standard deviation Aug 2, 2023
@codecov
Copy link

codecov bot commented Aug 2, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.01% ⚠️

Comparison is base (f0cc984) 99.71% compared to head (9f472a8) 99.71%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3226      +/-   ##
===========================================
- Coverage    99.71%   99.71%   -0.01%     
===========================================
  Files          248      248              
  Lines        18826    18824       -2     
===========================================
- Hits         18772    18770       -2     
  Misses          54       54              
Files Changed Coverage Δ
pybamm/parameters/geometric_parameters.py 100.00% <ø> (ø)
pybamm/parameters/lithium_ion_parameters.py 100.00% <ø> (ø)
pybamm/parameters/size_distribution_parameters.py 100.00% <ø> (ø)

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

@jeromtom
Copy link
Contributor Author

jeromtom commented Aug 2, 2023

The linkChecker has failed because of a timeout from loading/checking one website.

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.

Looks good, thanks! Can you also remove the mean and standard deviation parameters where they are added to the parameter set in size_distribution_parameters.py?

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.

Thanks!

@jeromtom
Copy link
Contributor Author

jeromtom commented Aug 3, 2023

I think I also have to remove the tests that search for the removed parameter to fix the build checks.
@tinosulzer Kindly confirm

@valentinsulzer
Copy link
Member

Yes, you need to replace them with a test for a different parameter that should be added by size_distribution_parameters (e.g. the minimum particle radius)

@jeromtom
Copy link
Contributor Author

jeromtom commented Aug 5, 2023

Unsuccessful checks:

  1. linkChecker: Timeout with the link https://www.rse.ox.ac.uk/ in README.md
  2. benchmarks on PR: Originates from a previously merged PR
  3. nox test in Ubuntu 3.10: This PR does not update the requirements of nox or PyBaMM. If the nox tests were recently updated we have to update the hashes also.

@valentinsulzer valentinsulzer merged commit 4288a72 into pybamm-team:develop Aug 7, 2023
17 of 19 checks passed
@jeromtom jeromtom deleted the Issue#3219-remove-particle-size-parameters-mean-sd branch August 7, 2023 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove particle size parameters: mean, standard deviation
2 participants