-
Notifications
You must be signed in to change notification settings - Fork 85
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
Battery Life Model for LMO/LTO chemistry #588
Conversation
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 love the new code structure. Questions & revisions mostly deal with areas that need improved comments.
std::shared_ptr<lifetime_nmc_state> nmc_li_neg; | ||
|
||
// LMO/LTO model state | ||
std::shared_ptr<lifetime_lmolto_state> lmo_lto; |
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 you add a comment about which of these should be used and which should be null for each model? It's a little confusing that the cycle model pointer here isn't actually used for the nmc and lmo/lto models - is there a way to break up some of the cycle counting functions and cycle degradation functions? (that can be a different PR if needed)
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.
That's a good point. In the current code, all these shared_ptrs are not NULL even when they aren't being used, such as the lmolto pointer when using an nmc model. I added comments but also the ability to create only the sub-state structs that will be used.
ssc/shared/lib_battery_lifetime.cpp
Line 50 in cc40cae
if (model_choice == lifetime_params::CALCYC) { |
That'll make it cleaner, and hopefully also clear up a little bit how the cycle model pointer here relates to the one used within the NMC & LMO/LTO models. Specifically, they are the same pointer.
ssc/shared/lib_battery_lifetime_nmc.cpp
Line 32 in cc40cae
cycle_model = std::unique_ptr<lifetime_cycle_t>(new lifetime_cycle_t(params, state)); |
The state created here has a cycle state that then gets passed to the constructor of the cycle model.
I'm not sure what you meant by breaking up the cycle counting functions?
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 have much to add. Looks good!
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.
Thanks for updating the comments and constructors!
Beta version of a battery life model for LMO/LTO chemistry. The model is based on test data being collected as part of BTMS, and is in-progress as the data collection is still ongoing. The final version will be similar in structure to the version in this PR, and the results will probably not change a lot. It will be published in a paper, so this model needs to be updated in the future with the reference also.
I'd like to include this version in SAM right now, in develop, in order for it to be used in PySAM for BTMS, as we're planning to start using this beta version for some analyses.
Changes:
LMOLTO
option to cmod_battery, cmod_pvsamv1, and cmod_batterystatefulNRELNMC
toNMC
for simplicitytest/input_cases/battery_lmolto_life/lifetime_validation_cell_1.json
contains a dictionary for running the life model and expected outputs for 16 different test conditions