-
-
Notifications
You must be signed in to change notification settings - Fork 546
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
[Bug]: hysteresis state error #4332
Comments
@llh929423719 can you provide more details on the specific base parameter set, the experimental protocol, the model used, and the solver? I would like to try and replicate the error you are experiencing. |
paramset:
testbench
|
After reading the literature, I wondered if it was due to the current problem. I modified the current calculation method and it seemed to be improved. The following is my modified wycisk_ocp(mainly modified set_rhs):
|
@llh929423719 Could you explain what your changes mean in mathematical terms? I see that you corrected some of the unit peculiarities for the implied units of the @mleot I was working with this submodel recently (thank you for the very helpful addition!). The Plett model where @llh929423719 I notice that you also changed the initial condition for |
@ejfdickinson The formula in the Wycisk paper is |
Thanks @llh929423719 - I think your approach makes the most sense so that Ultimately your change just alters the numerical value of the decay rate though, rather than changing the structure of the equations. So you could achieve the same just be scaling the decay rate, right? Do you see the same improvement if you keep the 24.5 release code and scale the decay rate parameter magnitude? |
Thanks @ejfdickinson , keeping the 24.5 release code and scale the decay rate parameter magnitude has the same effect. |
Hi @ejfdickinson, thanks for these comments. You make a fair point - as I was writing the code for this, I realized that realistically the current which is impacting the hysteresis should be measured locally at the particle interface, instead of a lumped electrode-level current. I believe I made an oversight by not reflecting and adjusting the units accordingly. I think there could be an argument for having K be a united quantity, although I have not thought about it enough to make any arguments there. Further, I do like the suggestion of being able to specify the initial hysteresis state condition in the parameter set. @llh929423719 I think the modification that you made above is not a bad idea. My only criticism would be that I don't believe the electrode thickness needs to be included in the calculation. I believe the alternate form below would make sure K is unitless.
I suspect the issue you encountered may have been from modifying the initial hysteresis state to a value very close to it's limit (I don't believe the hysteresis state variable should every actually reach 1 or -1). In the original bug, did you modify the initial condition at all? |
@mleot thanks for reply, I modify the initial condition just for fit the experiment data (cell is fully charged before discharge), but setting initial condition to zero does not make improvement. I take the electrode thickness into calculation just because the |
To keep electrode thickness out and to keep the focus on particle-scale properties, would it be better to normalise to the volumetric charge capacity of the pure material instead? That is: This means that the formulation is considering the particle-scale ratio of current to charge capacity. This is directly compatible with the need to normalise only by the capacity of the particle phase under consideration. At present you are getting a whole electrode property (product of volumetric charge capacity with electrode dimensions) and then having to divide back out by the electrode dimensions to get back to the volumetric charge capacity. So just grab the voilumetric charge capacity directly? edit: removed discussion of single-phase vs total capacity which is already handled in the current code |
I’m in favour of updating this formulation. It will be helpful to have reusable and easily interpretable parameters, especially as we look to integrate more hysteresis models. |
@rtimms Would it be worth opening a discussion for this? I've used the |
I’m happy to use this issue to discuss the changes. Could you post your suggestions and we can figure out a plan to move forward? |
Suggestions for improvements:
for
where the evaluation at a reference point (implicitly at
|
@ejfdickinson Thanks for the suggestions. Regarding your first suggestion: I think is a neat implementation of this portion of the equation. Regarding the second suggestion: Is this specific equation referenced in the Wycisk paper? This does seem like a better way to implement the differential capacity with the power term. How do you suggest the reference point be implemented? An additional parameter for a specific stoichiometry from which the reference value is extracted? I also like the idea of being able to specify a custom function, but the implementation for that seems potentially challenging. It might be nice in an alternate implementation to have a The last suggestion makes sense and I have no arguments against it. |
@mleot I don't think this specific equation is anywhere in Wycisk but it's just a factorisation of a scalar into the product of two parts with the effect of (a) tidying up the units and (b) correspondingly making more explicit and comprehensible the physical meaning of the parameters. Wycisk et al. would be far from the first authors to fail to introduce the tidiest mathematical expression of their own good idea, particularly when a general-purpose modelling approach is envisaged. Setting the specific stoichiometry at which the reference value is extracted is sensible to me. That just changes the definition of |
I don't completely know enough about PyBaMM's architecture to know the difficulties of arbitrary funtional input for At least a scalar input option would be very useful as an 'opt-out' from the specific Wycisk relation. I have some models at the moment where I achieve scalar input by setting |
Referring to @rtimms for advice on difficulty of implementation |
If you make something a An alternative approach would be to have a general model that allows for scalar In that case, we could have a general |
@rtimms @mleot I guess in the first instance we would expect this to be parameterised depending on lithiation extent (cs / csmax) and temperature, like diffusivity is. Anything more would be in the realm of future custom submodels depending on academic developments in understanding of the physical phenomena that dictate the magnitude of |
Sounds good! Would either of you like to take this on? IMO we should just do it as a breaking change and make sure we document how to update parameters that users have for the current implementation to match the new one. |
I would recommend that @mleot continues actual development of this class (if he is willing!), but I would be happy to assist with code review and testing. |
If helpful, I’m happy to help with setting up the new class structure. Then I’ll leave the details to @mleot (again, if you’re willing!). |
I would be happy to help! @rtimms If you want to set up the new class structure that would be very helpful! |
PyBaMM Version
24.5
Python Version
3.9
Describe the bug
Under some specific parameter sets, hysteresis state will exceed the range of (-1,1),adding bounds in WyciskOpenCircuitPotential submodel seems to have no effect.
Steps to Reproduce
my hysteresis paramset:
'Secondary: Negative particle hysteresis decay rate': 0.005,
'Secondary: Negative particle hysteresis switching factor': 0.2,
Relevant log output
No response
The text was updated successfully, but these errors were encountered: