-
Notifications
You must be signed in to change notification settings - Fork 24
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
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #149 +/- ##
===========================================
+ Coverage 93.03% 94.10% +1.06%
===========================================
Files 34 36 +2
Lines 1178 1323 +145
===========================================
+ Hits 1096 1245 +149
+ Misses 82 78 -4 ☔ View full report in Codecov by Sentry. |
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.
Hi @NicolaCourtier, I've start the review but have to cut it short. I'll try to continue to it later this afternoon. Great addition, I'm really excited for it to be merged! I've added comments around areas that I think can be improved or where I didn't follow the flow.
examples/scripts/spme_max_energy.py
Outdated
|
||
|
||
# Define functions | ||
def nominal_capacity(x, 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.
I think I would prefer to have this be within a pybop class. This example would be simpler with nominal_capacity()
, cell_mass()
, and GravimetricEnergyDensity()
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 the nominal_capacity
and cell_mass
methods inside something like a _utilities.py
file. They could be then be called within this example via pybop.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!
examples/scripts/spme_max_energy.py
Outdated
theoretical_energy = model._electrode_soh.calculate_theoretical_energy( | ||
model._parameter_set | ||
) | ||
average_voltage = ( |
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.
examples/scripts/spme_max_energy.py
Outdated
""" | ||
|
||
# Approximations due to SPM(e) parameter set limitations | ||
electrolyte_density = ps["Separator density [kg.m-3]"] |
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.
|
||
|
||
# Define cost function as a subclass | ||
class GravimetricEnergyDensity(pybop.BaseCost): |
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.
@@ -259,6 +259,12 @@ def __init__( | |||
init_soc=self.init_soc, | |||
) | |||
|
|||
# Add an example dataset for plotting comparison |
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'm not sure I follow this addition. Could this be completed outside of the DesignProblem
class, maybe by grabbing the first step in the optimiser?
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.
Possibly? It could be left to the user to add these steps if desired. I added them here simply to ensure that there is a target dataset for the plotting comparisons.
…t userwarning, reduce iterations for tests
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.
Looks good @NicolaCourtier, nice job!
No description provided.