Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add design optimisation example #149
Add design optimisation example #149
Changes from 16 commits
18c77d2
1cd6732
22526a9
e55eb81
985fc19
40ec760
d45463c
5e5c85e
30520f9
c444690
b7d3125
85ec240
546a1be
9384bc7
97606f0
7c9bb44
5d0c60f
77a66d9
f1ec4bc
5b0335b
9247fa1
a53b3e1
b900110
2578c36
112139d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I think I would prefer to have this be within a pybop class. This example would be simpler with
nominal_capacity()
,cell_mass()
, andGravimetricEnergyDensity()
in a seperate pybop class.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.
For now, they need to be defined in the example - the definitions are not fixed and have been adjusted for the purpose of maximising the gravimetric energy density within the constraints of the model. Do you mean a class within the script?
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.
More like the
GravimetricEnergyDensity
class would be within pybop.costs with thenominal_capacity
andcell_mass
methods inside something like a_utilities.py
file. They could be then be called within this example viapybop.cell_mass()
andpybop.nominal_capacity()
, without needing to be defined.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.
As below, I don't think the definitions should be taken out of context at this stage, but I think this example is still worth having as we continue to test and develop!
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.
Should this be the mean of the output model voltage instead of the mean on the bounds? A mean value from the OCV function is an alternative if we don't have the model output at this point.
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.
The nominal capacity is required to scale the C-rate used in the experiment, so it must be computed before the simulation. However, it is only a scaling factor on the current so, as long as the same
average_voltage
is used each time, I believe the energy density will be maximised.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.
Understood. This implementation would result in an optimal energy density for a given applied current (scaled by the calculated nom. capacity), but this applied current wouldn't be the correct C-rate, right? I think we can get the correct mean voltage via the OCV function though.
Something like this
model._parameter_set["Positive electrode OCP [V]"](mean_sto)
where themean_sto
variable can be calculated by taking the mean ofmodel._esoh_solver.get_min_max_stoichiometries()
. More information here.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.
As far as I understand, there's no feasible way to compute the "correct C-rate" - this is a challenge that comes with using C-rate instead of a defined current and one that we will probably have to address more explicitly in user guidance in the future. In practice, the measured charge/discharge capacity depends on the electrode balancing as well as the overpotential dynamics. So the question becomes: how accurate does the C-rate need to be for the optimisation results to hold and by how much do we want to prioritise accuracy over speed?
The approach you suggest will certainly give a more accurate "average_voltage" but it will also increase the computational load, which is already high due to the re-building. It may also disguise the fact that we need to use an approximation here to obtain a C-rate pre-experiment. We could calculate the realistic mean once at the beginning, using the starting parameter set and inputs, but this would change the cost values from run-to-run so reproducibility could be an issue.
A fixed, easy-to-compute value avoids these issues and, while a particular voltage use-case may be more helpful, I think an average of the voltage limits is a reasonable approximation for now.
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.
No electrolyte density variable for the SPMe, correct? Since it's just a scalar in this example, I would probably remove it instead of substituting with the separator density variable.
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.
Alternatively, we could just pull from literature for a standard EMC/DMC elecrolyte at a given molarity.
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.
Can this be within _costs.py?
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.
I don't think it should be taken out of context at this stage.
Check warning on line 146 in examples/scripts/spme_max_energy.py
Codecov / codecov/patch
examples/scripts/spme_max_energy.py#L146