-
-
Notifications
You must be signed in to change notification settings - Fork 563
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
Ecm with diffusion #4254
Ecm with diffusion #4254
Conversation
Thanks! Is there any reference for this in the literature? |
Hi. The used references are in the following. We have validated the results with an implementation of Chebyshev Collocation method developed by separate Python code:
The address for the developed Python code, used to validate SoC profile is: |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4254 +/- ##
===========================================
- Coverage 99.50% 99.46% -0.05%
===========================================
Files 288 289 +1
Lines 22079 22146 +67
===========================================
+ Hits 21970 22027 +57
- Misses 109 119 +10 ☔ View full report in Codecov by Sentry. |
pybamm/models/submodels/equivalent_circuit_elements/diffusion_element.py
Show resolved
Hide resolved
@MehrdadBabazadeh thanks for sharing the references, can you document these as described here? https://github.com/pybamm-team/PyBaMM/blob/develop/CONTRIBUTING.md#citations Also, you can link that git repository in the docstring of your submodel. These references will be useful in future if someone wants to know more about the model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last thing now that #4266 was created to address set_events
being misnamed.
pybamm/models/submodels/equivalent_circuit_elements/diffusion_element.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems OK now as long as the tests pass
@MehrdadBabazadeh Are you still working on the coverage for this? |
Yes. Ferran and me will go on the remaining issues and continue the work next week. |
The coverage issues seem to be unrelated to this PR, so that should be good to go. Waiting to merge as I don't know if we need to wait for #4311 |
@brosaplanella, we can merge this one without waiting for #4311. Coverage failure seems unrelated to me too as giving failure in |
@all-contributors please add @MehrdadBabazadeh for code and tests |
I've put up a pull request to add @MehrdadBabazadeh! 🎉 |
Thanks to @brosaplanella and other team members! |
* add diffusion element for ECM * fix bugs * An example to compare ECM with ECMD model * style: pre-commit fixes * We removed the example to compare two models. * Added citation. * Fixed typo. * Added entry to change log. * style: pre-commit fixes * Added test * style: pre-commit fixes * Fixed citation error. * Changed line 39. * New test_default_properties definition added. * style: pre-commit fixes * Increased cover * style: pre-commit fixes --------- Co-authored-by: Ferran Brosa Planella <[email protected]> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Eric G. Kratz <[email protected]> Co-authored-by: Agriya Khetarpal <[email protected]>
Description
The existing Equivalent Circuit Model (ECM) has been extended to create a more accurate model named the ECMD (ECM+Diffusion). This improvement involved adding a "diffusion_element" to the circuit alongside updates to the voltage_model and other relevant files. The key difference lies in how terminal voltage is calculated. The original ECM relied on average State of Charge (SoC). With the diffusion option enabled, the ECMD utilizes a newly calculated Surface SoC, providing a more precise representation. Additionally, a new term, eta_diffusion, is introduced. This term represents the voltage drop caused by diffusion limitations and is placed in series with the existing Open Circuit Voltage (OCV) within the circuit. This approach isolates the pure effect of diffusion on the voltage calculations. Finally, a Python script named Compare_ECM_ECMD.py is included as an example to demonstrate the comparison between the original ECM and the new ECMD.
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.
Key checklist:
$ 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)$ python run-tests.py --all
(or$ nox -s tests
)$ 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: