RMG-Electrochem

[mjohnson541]

Enables RMG to operate on solid electrolyte interface conditions in Li-ion batteries. Adds Li atoms, adds electrochemical reactions, adds a liquid-catalysis reactor and upgrades tree generation adding corrections for kinetic solvent effect.

[lgtm-com]

This pull request introduces 5 alerts when merging 10314f2 into 2a35d62 - view on LGTM.com

new alerts:

• 2 for Except block handles 'BaseException'
• 1 for Unused import
• 1 for Syntax error
• 1 for Unreachable code

[lgtm-com]

This pull request introduces 5 alerts when merging 4f6b5db into e3d0617 - view on LGTM.com

new alerts:

• 2 for Except block handles 'BaseException'
• 1 for Unused import
• 1 for Syntax error
• 1 for Unreachable code

[rwest]

I rebased, did a few interactive rebases to clean up some fixups and merge conflict markers that had gotten in there, and force pushed.

[rwest]

The database tests are failing, because some sample molecules are being made that are charged.

In family 1,2_Insertion_CO, a sample molecule made from node COS returns an unexpectedly charged molecule:
Sample molecule AdjList:
1 *4 O u0 p1 c+1 {2,T}
2 *1 C u0 p1 c-1 {1,T}


Origin Group AdjList:
1 *1 C     u0 p1 c-1 {2,T}
2 *4 [O,S] u0 p1 c+1 {1,T}

ERROR:root:
In family 1,2_Insertion_CO, a sample molecule made from node CO returns an unexpectedly charged molecule:
Sample molecule AdjList:
1 *4 O u0 p1 c+1 {2,T}
2 *1 C u0 p1 c-1 {1,T}


Origin Group AdjList:
1 *1 C u0 p1 c-1 {2,T}
2 *4 O u0 p1 c+1 {1,T}

ERROR:root:
In family 1,2_Insertion_CO, a sample molecule made from node CS returns an unexpectedly charged molecule:
Sample molecule AdjList:
1 *4 S u0 p1 c+1 {2,T}
2 *1 C u0 p1 c-1 {1,T}


Origin Group AdjList:
1 *1 C u0 p1 c-1 {2,T}
2 *4 S u0 p1 c+1 {1,T}

ERROR

Kinetics family 1,2_NH3_elimination: Entry is accessible? ... ERROR:root:
In family 1,2_NH3_elimination, a sample molecule made from node charged returns an unexpectedly charged molecule:
Sample molecule AdjList:
1 *2 N u0 p0 c+1 {2,S} {3,S} {4,S} {5,S}
2 *1 N u0 p1 c0 {1,S} {6,S} {7,S}
3 *3 N u0 p2 c-1 {1,S} {8,S}
4 *4 H u0 p0 c0 {1,S}
5    H u0 p0 c0 {1,S}
6    H u0 p0 c0 {2,S}
7    H u0 p0 c0 {2,S}
8    H u0 p0 c0 {3,S}


Origin Group AdjList:
1 *1 N u0 p1 c0 {2,S} {5,S} {6,S}
2 *2 N u0 p0 c+1 {1,S} {3,[S,D]} {4,S}
3 *3 N u0 p2 c-1 {2,[S,D]}
4 *4 H u0 p0 c0 {2,S}
5    H u0 p0 c0 {1,S}
6    H u0 p0 c0 {1,S}

ERROR:root:
In family 1,2_NH3_elimination, a sample molecule made from node chargedS returns an unexpectedly charged molecule:
Sample molecule AdjList:
1 *2 N u0 p0 c+1 {2,S} {3,S} {4,S} {5,S}
2 *1 N u0 p1 c0 {1,S} {6,S} {7,S}
3 *3 N u0 p2 c-1 {1,S} {8,S}
4 *4 H u0 p0 c0 {1,S}
5    H u0 p0 c0 {1,S}
6    H u0 p0 c0 {2,S}
7    H u0 p0 c0 {2,S}
8    H u0 p0 c0 {3,S}


Origin Group AdjList:
1 *1 N u0 p1 c0 {2,S} {5,S} {6,S}
2 *2 N u0 p0 c+1 {1,S} {3,S} {4,S}
3 *3 N u0 p2 c-1 {2,S}
4 *4 H u0 p0 c0 {2,S}
5    H u0 p0 c0 {1,S}
6    H u0 p0 c0 {1,S}

ERROR:root:
In family 1,2_NH3_elimination, a sample molecule made from node chargedD returns an unexpectedly charged molecule:
Sample molecule AdjList:
1 *1 N u0 p1 c0 {2,S} {5,S} {6,S}
2 *2 N u0 p0 c+1 {1,S} {3,D} {4,S}
3 *3 N u0 p2 c-1 {2,D}
4 *4 H u0 p0 c0 {2,S}
5    H u0 p0 c0 {1,S}
6    H u0 p0 c0 {1,S}


Origin Group AdjList:
1 *1 N u0 p1 c0 {2,S} {5,S} {6,S}
2 *2 N u0 p0 c+1 {1,S} {3,D} {4,S}
3 *3 N u0 p2 c-1 {2,D}
4 *4 H u0 p0 c0 {2,S}
5    H u0 p0 c0 {1,S}
6    H u0 p0 c0 {1,S}

ERROR
Kinetics family 1,2_NH3_elimination: Recipe applies to group entry? ... ERROR

Kinetics family R_Addition_CSm: Entry is accessible? ... ERROR:root:
In family R_Addition_CSm, a sample molecule made from node CSm returns an unexpectedly charged molecule:
Sample molecule AdjList:
1 *3 S u0 p1 c+1 {2,T}
2 *1 C u0 p1 c-1 {1,T}


Origin Group AdjList:
1 *3 S u0 p1 c+1 {2,T}
2 *1 C u0 p1 c-1 {1,T}

ERROR
Kinetics family R_Addition_CSm: Recipe applies to group entry? ... ERROR

Kinetics family Surface_Adsorption_vdW: Entry is accessible? ... ERROR:root:
In family Surface_Adsorption_vdW, a sample molecule made from node N2O returns an unexpectedly charged molecule:
Sample molecule AdjList:
1 *1 O u0 p2 c0 {2,D}
2    N u0 p0 c+1 {1,D} {3,D}
3    N u0 p2 c-1 {2,D}


Origin Group AdjList:
multiplicity [1]
1    N u0 p0 c+1 {2,D} {3,D}
2 *1 O u0 p2 c0 {1,D}
3    N u0 p2 c-1 {1,D}

ERROR
Kinetics family Surface_Adsorption_vdW: Recipe applies to group entry? ... ERROR

Solvation groups group: Entry is accessible? ... ERROR:root:Problem making sample molecule for group Cs-N5cHHH
1 * Cs u0 {2,S} {3,S} {4,S} {5,S}
2   N  u0 c+1 {1,S}
3   H  u0 {1,S}
4   H  u0 {1,S}
5   H  u0 {1,S}

ERROR:root:Problem making sample molecule for group Cs-N5cCsHH
1 * Cs u0 {2,S} {3,S} {4,S} {5,S}
2   N  u0 c+1 {1,S}
3   Cs u0 {1,S}
4   H  u0 {1,S}
5   H  u0 {1,S}

ERROR:root:
In family group, a sample molecule made from node Cs-N5cHHH returns an unexpectedly charged molecule:
Sample molecule AdjList:
1   N u0 p1 c+1 {2,S} {6,S}
2 * C u0 p0 c0 {1,S} {3,S} {4,S} {5,S}
3   H u0 p0 c0 {2,S}
4   H u0 p0 c0 {2,S}
5   H u0 p0 c0 {2,S}
6   H u0 p0 c0 {1,S}


Origin Group AdjList:
1 * Cs u0 {2,S} {3,S} {4,S} {5,S}
2   N  u0 c+1 {1,S}
3   H  u0 {1,S}
4   H  u0 {1,S}
5   H  u0 {1,S}

ERROR:root:
In family group, a sample molecule made from node Cs-N5cCsHH returns an unexpectedly charged molecule:
Sample molecule AdjList:
1   N u0 p1 c+1 {2,S} {9,S}
2 * C u0 p0 c0 {1,S} {3,S} {4,S} {5,S}
3   C u0 p0 c0 {2,S} {6,S} {7,S} {8,S}
4   H u0 p0 c0 {2,S}
5   H u0 p0 c0 {2,S}
6   H u0 p0 c0 {3,S}
7   H u0 p0 c0 {3,S}
8   H u0 p0 c0 {3,S}
9   H u0 p0 c0 {1,S}


Origin Group AdjList:
1 * Cs u0 {2,S} {3,S} {4,S} {5,S}
2   N  u0 c+1 {1,S}
3   Cs u0 {1,S}
4   H  u0 {1,S}
5   H  u0 {1,S}

ERROR

Thermo groups group: Entry is accessible? ... ERROR:root:Problem making sample molecule for group Sc
1 * S ux c+1

ERROR:root:
In family group, a sample molecule made from node Sc returns an unexpectedly charged molecule:
Sample molecule AdjList:
1 * S u0 p2 c+1 {2,S}
2   H u0 p0 c0 {1,S}


Origin Group AdjList:
1 * S ux c+1

ERROR

etc.

looks like they're probably mostly the same cause.
Any ideas @mjohnson541 ?

[mjohnson541]

Thanks for the help! I meant to get on this last weekend, but I've been ill and now I'm frantically trying to get some urgent calculations done. Hopefully I'll be working on this PR as soon as those launch.

At some point a month or so ago I think I figured out why that was happening, but I don't remember now. Skimming the sample molecule code doesn't seem to be jogging my memory.

[mjohnson541]

I think the differences in the core regressions for liquid runs look alright, the observable failure for RMS_CSTR_liquid_oxidation gave me a bit of a pause, but it looks like the observable for that isn't even for the same phase simulation as the run so I'm inclined to ignore it.

[github-actions]

This pull request is being automatically marked as stale because it has not received any interaction in the last 90 days. Please leave a comment if this is still a relevant pull request, otherwise it will automatically be closed in 30 days.

[JacksonBurns]

@mjohnson541 could I jump in and help get the conflicts resolved with the testing files? I can't review the content but can at least make it merge-able.

[mjohnson541]

Sure! That'd be super helpful! I should be back working on merging this in a couple weeks.

[JacksonBurns]

@mjohnson541 great! I will get the conflicts resolved before then, but probably by opening a PR into this branch from a separate branch so you can look at the changes.

Do you have any idea of how 'close' this PR is to done (besides the conflicts)?

[mjohnson541]

Awesome! Thanks so much! Apart from the conflicts it should be ready for review and merging.

[hwpang]

I think the differences in the core regressions for liquid runs look alright, the observable failure for RMS_CSTR_liquid_oxidation gave me a bit of a pause, but it looks like the observable for that isn't even for the same phase simulation as the run so I'm inclined to ignore it.

I don't understand. What do you mean by that the observable is not even for the same phase simulation as the run? RMS_CSTER_liquid_oxidation is essentially liquid_oxidation except as open system. There is only liquid phase. If you're talking about the regression simulation being run at gas phase but not liquid phase, that is what liquid_oxidation does as well. I don't think we should ignore the RMS regression tests.

[mjohnson541]

I think the differences in the core regressions for liquid runs look alright, the observable failure for RMS_CSTR_liquid_oxidation gave me a bit of a pause, but it looks like the observable for that isn't even for the same phase simulation as the run so I'm inclined to ignore it.

I don't understand. What do you mean by that the observable is not even for the same phase simulation as the run? RMS_CSTER_liquid_oxidation is essentially liquid_oxidation except as open system. There is only liquid phase. If you're talking about the regression simulation being run at gas phase but not liquid phase, that is what liquid_oxidation does as well. I don't think we should ignore the RMS regression tests.

In my opinion we should probably not in general be taking a model we built for liquid system A and then benchmarking it on gas phase system B. Philosophically, I think it's kind of like taking a fish out of water and measuring whether it climbs a tree as well as the last fish. Technically it tells you something about how different the fish is and a really bad fish is probably not going to be able to climb a tree well but it isn't what you optimized the fish for so it doesn't tell you if your fish is a better or worse fish in the water.

Outside of that, while I didn't elaborate on it before, I also don't think it's that surprising to see the concentration change by more than 10% for pentane given the changes in species/reaction selection due to the new solute data and the limited size of the model.

[hwpang]

I think the differences in the core regressions for liquid runs look alright, the observable failure for RMS_CSTR_liquid_oxidation gave me a bit of a pause, but it looks like the observable for that isn't even for the same phase simulation as the run so I'm inclined to ignore it.

I don't understand. What do you mean by that the observable is not even for the same phase simulation as the run? RMS_CSTER_liquid_oxidation is essentially liquid_oxidation except as open system. There is only liquid phase. If you're talking about the regression simulation being run at gas phase but not liquid phase, that is what liquid_oxidation does as well. I don't think we should ignore the RMS regression tests.

In my opinion we should probably not in general be taking a model we built for liquid system A and then benchmarking it on gas phase system B. Philosophically, I think it's kind of like taking a fish out of water and measuring whether it climbs a tree as well as the last fish. Technically it tells you something about how different the fish is and a really bad fish is probably not going to be able to climb a tree well but it isn't what you optimized the fish for so it doesn't tell you if your fish is a better or worse fish in the water.

Outside of that, while I didn't elaborate on it before, I also don't think it's that surprising to see the concentration change by more than 10% for pentane given the changes in species/reaction selection due to the new solute data and the limited size of the model.

I agree that the current way of doing it is not ideal and someone should go in and implement a liquid phase version of regression run. But since the current stable version was run with gas phase, theoretically the PR'd version run in gas phase should at least be consistent. It just doesn't make sense to say it's ok to do such comparison for the liquid_oxidation case, but not ok for the RMS_CSTR_liquid_oxidation case.

[mjohnson541]

I think the differences in the core regressions for liquid runs look alright, the observable failure for RMS_CSTR_liquid_oxidation gave me a bit of a pause, but it looks like the observable for that isn't even for the same phase simulation as the run so I'm inclined to ignore it.

I don't understand. What do you mean by that the observable is not even for the same phase simulation as the run? RMS_CSTER_liquid_oxidation is essentially liquid_oxidation except as open system. There is only liquid phase. If you're talking about the regression simulation being run at gas phase but not liquid phase, that is what liquid_oxidation does as well. I don't think we should ignore the RMS regression tests.

In my opinion we should probably not in general be taking a model we built for liquid system A and then benchmarking it on gas phase system B. Philosophically, I think it's kind of like taking a fish out of water and measuring whether it climbs a tree as well as the last fish. Technically it tells you something about how different the fish is and a really bad fish is probably not going to be able to climb a tree well but it isn't what you optimized the fish for so it doesn't tell you if your fish is a better or worse fish in the water.
Outside of that, while I didn't elaborate on it before, I also don't think it's that surprising to see the concentration change by more than 10% for pentane given the changes in species/reaction selection due to the new solute data and the limited size of the model.

I agree that the current way of doing it is not ideal and someone should go in and implement a liquid phase version of regression run. But since the current stable version was run with gas phase, theoretically the PR'd version run in gas phase should at least be consistent. It just doesn't make sense to say it's ok to do such comparison for the liquid_oxidation case, but not ok for the RMS_CSTR_liquid_oxidation case.

I'm confused, I don't think I ever said it was fine to do that in the liquid_oxidation case, but not the RMS_CSTR_liquid_oxidation case?

In general it is fine for us to have regression tests that primarily exist for us to register if a change has occurred. However, in those cases I think we should concern ourselves with the causal changes in parameter estimation rather than the observable itself.

[rwest]

Did this get stalled? I'm trying to interpret the comments above. Is someone resolving conflicts?

[JacksonBurns]

Did this get stalled? I'm trying to interpret the comments above. Is someone resolving conflicts?

Hi yes this is waiting on me. I will open a PR into this branch to get the tests fixed.

[rwest]

@mjohnson541 do you have any example input files for electrochemistry that you could include here?

@JacksonBurns what's the status with #2598 ? do we rebase that onto this, and then both onto main?

[JacksonBurns]

@JacksonBurns what's the status with #2598 ? do we rebase that onto this, and then both onto main?

I had intended that one to help with the rebasing, but as I mention in a comment there it might not be 'easy' still...

That branch is much closer to being compatible with main than this one - you could merge that one into this one, and then merge into main, or move development over there entirely. Not sure what the best path forward is.

[mjohnson541]

I have examples, but part of why they aren't on this branch yet is this isn't the complete PR, I didn't want to get in the way of the rebase, but I have the rest of the changes locally. I can try to get those added soon if we're ready.