Three electrode setups#38
Conversation
|
Why does this need to be special-cased in pybamm-eis, instead of using the 3-electrode capabilities from pybamm? |
|
So an option in PyBaMM instead? That would serve the same purpose and even more than that. |
|
I just checked the example on PyBaMM's three-electrode capabilities: https://github.com/pybamm-team/PyBaMM/blob/develop/docs/source/examples/notebooks/models/simulate-3E-cell.ipynb We could make it so "Voltage [V]" is not hard-coded, because that seems to be the issue here. |
|
This does almost the same and may even be useful to extract the shapes of individual components of the impedance, but it can not itself correct the sign or give a good initial condition anymore. The solver worked well with the blanket initial condition 0, but I have not tested whether it is slower. |
|
I'm in favor of allowing different outputs, but this seems like a strange implementation - it would be better to just allow the user to add some additional outputs to the model, than overwriting the voltage. |
|
I would agree if it weren't for the fact that the current implementation does not use the voltage variable directly. Instead, an extra equation mimicking the voltage variable (literally 0 = extra - voltage) is added on top. I assume that is done, so it is easier to track the index of the current and target variables. So I just made it customizable which extra variable gets added on top (0 = extra - target). |
|
Or would you rather have a list of "0 = extra - target" equations added to the equation stack before finishing it with the current and default voltage variables? Then I could make the retrieval logic for the extra variables, without breaking the hardcoded "[-2] equals current, [-1] equals voltage", which may be used somewhere else. |
|
I understand, given the existing implementation I think your existing approach is good |
| V_cell = pybamm.Variable("Voltage variable [V]") | ||
| new_model.variables["Voltage variable [V]"] = V_cell | ||
| V = new_model.variables["Voltage [V]"] | ||
| V = new_model.variables[target] | ||
| # Add an algebraic equation for the voltage variable | ||
| new_model.algebraic[V_cell] = V_cell - V | ||
| new_model.initial_conditions[V_cell] = new_model.param.ocv_init | ||
| new_model.initial_conditions[V_cell] = 0 |
There was a problem hiding this comment.
just rename these to be more generic then we are good to go
There was a problem hiding this comment.
i.e. target_variable instead of V_cell, "Target variable [V]" instead of "Voltage variable [V]", etc
There was a problem hiding this comment.
Done. I also added an example script.
2 participants
Solves issue #37