Add intermediate model classes#183
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## 162-update-design-optimisation-functionality #183 +/- ##
================================================================================
+ Coverage 93.30% 94.08% +0.77%
================================================================================
Files 35 37 +2
Lines 1345 1352 +7
================================================================================
+ Hits 1255 1272 +17
+ Misses 90 80 -10 ☔ View full report in Codecov by Sentry. |
|
Hi Nicola, Thanks for adding this, I think we should have an issue open to track this change moving forward. Can you open one for this PR? |
|
Hi Brady, I have created issue #186 to clarify the motivation for this PR - let me know if you have any further questions! |
BradyPlanden
left a comment
There was a problem hiding this comment.
Looks great @NicolaCourtier, thanks!
I've added a comment that I'm hoping will link and remind us to update the private reference to model in the problem class while merging #169. Otherwise just docstring updates.
| ], | ||
| "source": [ | ||
| "cost.nominal_capacity(x, cost.problem._model)\n", | ||
| "cost.problem._model.approximate_capacity(x)\n", |
There was a problem hiding this comment.
Future merge reminder: Update this using the model getter in #169
Co-authored-by: Brady Planden <55357039+BradyPlanden@users.noreply.github.com>
Co-authored-by: Brady Planden <55357039+BradyPlanden@users.noreply.github.com>
As discussed, here is an implementation of intermediate model classes to allow the additional model function required for design optiimisation to be included as part of an electrochemical base model - see what you think!
Regarding the nominal capacity estimate, I think it is reliable - I am happy for this to be included as a base echem function called approximate_capacity in combination with the user warnings I've added to the GravimetricEnergyDensity cost.