Issue 933 remove reactions dict #948
Conversation
There was a problem hiding this comment.
Looks good Tino, thanks! My only concern here (which I guess it is at the core of the whole PR) is how easy it is to add new reactions and how compatible it is with across different models. Won't we end up with a lot of reactions (and the subsequent variables) which in the standard DFN model they will all be zero?
Also, for modelling purposes and prototyping, do you think it would be useful to always have a "dummy or miscellaneous reaction" that can easily be defined to be whatever is needed, before hardcoding that as a new reaction in PyBAMM?
Finally, the pull request is to merge with issue-546-convection instead of develop. Is that intended?
| "Positive": {"s": 1, "aj": "Positive electrode" + icd}, | ||
| } | ||
| } | ||
| def set_other_reaction_submodels_to_zero(self): |
There was a problem hiding this comment.
instead of having set_other_reaction_submodels_to_zero could we have something like set_reaction_submodels(self,[list of reactions]): which then sets the appropriate models from the list (and sets NoReaction for things that aren't in the list)? I'm just thinking of how this extends in the future and works with model options. It would be nice to have an option "reactions" which takes a list of names of any reactions you want to include. Actually it looks like this already happens, and these are always zero for li-ion. Maybe there could be a comment or this method renamed, as seeing it in e.g. DFN was a little confusing -- it wasn't clear which reactions this referred to.
| ) | ||
| param = self.param | ||
| # All possible reactions. Some of these could be zero | ||
| j_av = variables["First-order x-averaged interfacial current density"] |
There was a problem hiding this comment.
it seems like there are a lot of places where you need to remember to add a new 'j'. Could we pass the list of reaction names in and then search for the side reactions that need to be added on? e.g. look for "First-order x-averaged" + "reaction_name" + "interfacial current density". Maybe there are some quirks that make this hard, but I'm worried people will forget to add the extra current densities
There was a problem hiding this comment.
I guess then you need things like s_plus values. And then we basically end up back at the reaction dictionary... I don't think I appreciated how often you needed to get each of the current densities in separate files
There was a problem hiding this comment.
Yeah exactly ... I'm leaning towards having to do everything explicitly though. I'll resolve conflicts and fix tests for now, and we can discuss at next meeting
There was a problem hiding this comment.
Maybe having "Total negative electrolyte reactions", "Total negative solid reactions", etc variables in the variables dictionary. The electrolyte one can include the s_plus values. When you add a new reaction, you then just do:
variables["Total electrolyte reactions"] += s_plus * j_new
If the reaction is set to NoReaction then we just don't update it. If the reaction only consumes a reactant from the electrolyte, then we only add it to the electrolyte and not the solid.
This would be a bit cleaner than the current reactions dictionary and also would make it clearer from just looking at a particular reaction submodel where that reaction fits into the wider model. Also good is that you only have to edit that one reaction submodel to control how that reaction is coupled to the full model. Downside is that it is less clear than the approach in this issue to see what reactions are taking place just by looking at the electrolyte submodel.
There was a problem hiding this comment.
Yeah, I think that's the cleanest way of doing it, then it just goes into BaseInterface and all happens automatically
@ferranbrosa can you explain what you mean by this? |
If I understand correctly every single reaction that we want to add needs to be defined correctly across a few files, even though the final form (or submodel) for the reaction might change. For example, the SEI formation will be there as a reaction but we might have different submodels for it that we can change. However, the SEI reaction is always there as it is a commonly accepted term. Now, it is likely that people will want to add a new reaction or experiment with it which would require the user to add the reaction in the right places. This could be quite messy, especially for new users. We could have a dummy or miscellaneous reaction, apart from all the commonly accepted reaction terms, which is normally off, but that can be switched on for prototyping. Once this reaction works, the issue of hardcoding it into PyBaMM can be posted. If I haven't understood it correctly, then the two paragraphs above probably make no sense haha |
I think the point is that if want to add a new side reaction submodel it's no longer self contained. In the sense that you have to also make changes to the electrode and electrolyte submodels . Maybe we could sum all these up in the interface submodel? i.e. define |
…3-remove-reactions-dict
…3-remove-reactions-dict
Codecov Report
@@ Coverage Diff @@
## develop #948 +/- ##
===========================================
- Coverage 97.49% 97.48% -0.02%
===========================================
Files 222 222
Lines 11773 11791 +18
===========================================
+ Hits 11478 11494 +16
- Misses 295 297 +2
Continue to review full report at Codecov.
|
Description
Remove the dictionary of reactions, and explicitly define reactions instead.
Fixes #933
Type of change
Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.
Key checklist:
$ flake8$ python run-tests.py --unit$ cd docsand then$ make clean; make htmlYou can run all three at once, using
$ python run-tests.py --quick.Further checks: