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
Reconciling Electrochem Branch with main
#2598
base: main
Are you sure you want to change the base?
Conversation
f8597e9
to
777e139
Compare
@rwest @mjohnson541 the remaining unit test failures (here: https://github.com/ReactionMechanismGenerator/RMG-Py/actions/runs/7750195947/job/21136056656#step:10:2072) seem related to the echem work, and not anything in this PR. Unfortunately (see below) we may have to move Unfortunately it seems that I have yet again misunderstood how on earth git works. I think if I had merged |
Thanks @JacksonBurns! This is great! I should be able to consolidate this with the last parts of echem development I have locally soon and then it should be all together for review. |
I rebased this onto the latest main, and changed the base (target) for the PR back to main. Also rebased the |
@@ -219,6 +219,30 @@ def get_sticking_coefficient(self, T): | |||
|
|||
return False | |||
|
|||
def apply_solvent_correction(self, solvent): |
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.
Has all of this solvent corrections to kinetics been done on a different branch (and reviewed independently?) or is it all blurred in with the electrochemistry stuff?
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.
So I had a solvation correction system I built for the electrochemistry work. Yunsie was also working on solvation corrections and had her own dataset and a system that was similar, but also a bit different. So I modified my system to merge the two systems and handle both my dataset and her dataset.
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.
Did hers never make it into RMG then? this is the [only] chance to review both?
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.
Yes, this is all only here.
I just added the additional changes on top of this branch. |
I have also updated the database branch. |
I just rebased it, did a bunch of interactive rebasing to rationalize things, group together commits that are related, and in some cases squash them. I notice that commit 655e8de "add Li adsorption to test data" (May 9, 2023) adds data to rmgpy/data/test_data which has now (as of #2644) been removed, so that commit (and maybe others that look for that data) will need editing. |
and Rx!H atom types
remove this commit before merging
Now includes checks for negative charges
This PR is related to #2316, which is very out of date with
main
(especially since #2380).I have already made on branch off
electrochem
and rebased it ontomain
. While doing so, I kept all of the testing files in their old format, as edited during the development onelectrochem
. I will start by opening this PR againstmain
to get the tests fixed and the branch 'merge-able', and then I will change this PR to be againstelectrochem
. We can then review to ensure that the rebase and testing changes are correct, merge intoelectrochem
, and thenelectrochem
can finish development and be merged.