RMG-Py icon indicating copy to clipboard operation
RMG-Py copied to clipboard

Reconciling Electrochem Branch with `main`

Open JacksonBurns opened this issue 1 year ago • 9 comments

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 onto main. While doing so, I kept all of the testing files in their old format, as edited during the development on electrochem. I will start by opening this PR against main to get the tests fixed and the branch 'merge-able', and then I will change this PR to be against electrochem. We can then review to ensure that the rebase and testing changes are correct, merge into electrochem, and then electrochem can finish development and be merged.

JacksonBurns avatar Feb 01 '24 15:02 JacksonBurns

@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 electrochem development to this branch (and change base back to main), or else reset electrochem to the latest commit on this branch so it looks like the rebase was done on electrochem directly?

Unfortunately it seems that I have yet again misunderstood how on earth git works. I think if I had merged main into this branch, we could have then merged this branch into the electrochem branch, and then the electrochem branch into main. The way I have done it here has basically stacked all the commits to main from before the last time electrochem was rebased, and then the commits on electrochem, and then my commits to make the tests work. I guess the history would look wacky if I did it not this way?

JacksonBurns avatar Feb 02 '24 02:02 JacksonBurns

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.

mjohnson541 avatar Feb 02 '24 22:02 mjohnson541

I rebased this onto the latest main, and changed the base (target) for the PR back to main. Also rebased the lithium branch in the RMG-database which this refers to. Let's see how the tests do...

rwest avatar Mar 08 '24 20:03 rwest

I just added the additional changes on top of this branch.

mjohnson541 avatar Mar 10 '24 02:03 mjohnson541

I have also updated the database branch.

mjohnson541 avatar Mar 10 '24 02:03 mjohnson541

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 655e8ded2d9181b6313dd4b510616876ad9f216f "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.

rwest avatar Apr 01 '24 19:04 rwest