Added contact resistance #2598
Added contact resistance #2598
Conversation
|
The BPX test is failing because contact resistance is not included in the BPX schema. If everything else about this PR is fine, I can raise an issue on the BPX Github. But I can see why you might be concerned about my having created separate variables for voltage with/without contact resistance. |
|
IMO the contact resistance parameter should be optional (i.e. add an option where it's not used and hence not needed). |
| if "Current [A]" in variables: | ||
| I = variables["Current [A]"] | ||
| V_contact_dim = V_dim - I * param.R_contact | ||
| elif self.options["operating mode"] == "explicit power": |
There was a problem hiding this comment.
In this case, wouldn't P=I*V_contact? And similar for explicit resistance? As in, the measured power would include the contact resistance? Or am I wrong?
It seems to me the cleanest way to do this would be to have a "contact resistance" option (so that the parameter is optional as @tinosulzer suggests) and then not allow "contact resistance" and "explicit power/resistance"?
| @@ -34,6 +34,7 @@ def setUp(self): | |||
| "in parallel to make a cell": 34, | |||
| "External surface area [m2]": 3.79e-2, | |||
| "Volume [m3]": 1.28e-4, | |||
| "Contact resistance [Ohm]": 0, | |||
There was a problem hiding this comment.
Remove this from here and the test will pass. BPX doesn't let you add extra fields that aren't in the schema, so it throws an error. You already hard-code zero contact resistance in bpx.py. If users want to read in BPX and add the contact parameter resistance after, they can do that in the usual way.
|
All of the above issues have now been resolved but it's now failing to build scikits-odes... |
|
Yes, @martinjrobins is working on fixing the scikits issue (not related to your changes) |
|
If you merge the latest version of develop it should work now |
Codecov ReportBase: 99.69% // Head: 99.69% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #2598 +/- ##
========================================
Coverage 99.69% 99.69%
========================================
Files 271 271
Lines 19560 19573 +13
========================================
+ Hits 19500 19513 +13
Misses 60 60
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| @@ -151,8 +156,8 @@ def _get_standard_current_collector_potential_variables(self, phi_s_cn, phi_s_cp | |||
| "Positive current collector potential [V]": phi_s_cp_dim, | |||
| "Local voltage": V_cc, | |||
| "Local voltage [V]": self.param.ocv_ref + V_cc * pot_scale, | |||
| "Terminal voltage": V, | |||
| "Terminal voltage [V]": V_dim, | |||
| "Terminal voltage": V - delta_phi_contact, | |||
There was a problem hiding this comment.
why is this minus and not plus?
There was a problem hiding this comment.
also would be good to add contact resistance as its own variable
There was a problem hiding this comment.
"Contact resistance [Ohm]" is a parameter. Did you mean make "Contact overpotential [V]" a variable? I can do that if you think it's appropriate.
There was a problem hiding this comment.
Yes I meant contact overpotential as a variable. Also smart to ignore my dumb minus vs plus comment, brain fart on my part
Description
A new parameter "Contact resistance [Ohm]" has been added. This allows for the creation of a new variable "Contact voltage [V]", which is the same as "Terminal voltage [V]" but corrects for said contact resistance.
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:
$ flake8$ python run-tests.py --unit$ cd docsand then$ make clean; make htmlYou can run all three at once, using
$ python run-tests.py --quick.Further checks: