Closed JacksonBurns closed 5 days ago
@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?
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 lithium
branch in the RMG-database which this refers to. Let's see how the tests do...
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 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.
cat: write error: Resource temporarily unavailable WARNING:root:Initial mole fractions do not sum to one; normalizing. cat: write error: Resource temporarily unavailable WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. ⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
cat: write error: Resource temporarily unavailable WARNING:root:Initial mole fractions do not sum to one; normalizing. cat: write error: Resource temporarily unavailable WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. ⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
cat: write error: Resource temporarily unavailable WARNING:root:Initial mole fractions do not sum to one; normalizing. cat: write error: Resource temporarily unavailable WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. ⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
cat: write error: Resource temporarily unavailable WARNING:root:Initial mole fractions do not sum to one; normalizing. cat: write error: Resource temporarily unavailable WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. ⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
cat: write error: Resource temporarily unavailable WARNING:root:Initial mole fractions do not sum to one; normalizing. cat: write error: Resource temporarily unavailable WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. ⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
cat: write error: Resource temporarily unavailable WARNING:root:Initial mole fractions do not sum to one; normalizing. cat: write error: Resource temporarily unavailable WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. ⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
cat: write error: Resource temporarily unavailable WARNING:root:Initial mole fractions do not sum to one; normalizing. cat: write error: Resource temporarily unavailable WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. ⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
cat: write error: Resource temporarily unavailable WARNING:root:Initial mole fractions do not sum to one; normalizing. cat: write error: Resource temporarily unavailable WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. ⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
cat: write error: Resource temporarily unavailable WARNING:root:Initial mole fractions do not sum to one; normalizing. cat: write error: Resource temporarily unavailable WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. ⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
In order to get this in we should either tell everyone to use the marcus_development branch of RMS by tweaking the installation instructions like this
...or merge that branch to main on RMS first. Which is https://github.com/ReactionMechanismGenerator/ReactionMechanismSimulator.jl/pull/267
cat: write error: Resource temporarily unavailable WARNING:root:Initial mole fractions do not sum to one; normalizing. cat: write error: Resource temporarily unavailable WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. ⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
Hi all, @rwest @JacksonBurns @mjohnson541
After some discussion with Richard, we laid out the following list of actions for deploying this PR and its sister PR on RMG-database:
lithium_rebase
branch of RMG-database passes our CI workflow, with the echem_rebase
branch of RMG-Py and marcus_development
branch of RMS. echem-rebase
branch of RMG-Py passes our CI workflow, with the lithium_rebase
branch of RMG-database and `marcus_development branch of RMS. lithium_rebase
back to using main
branch of RMG-Py. This will make the CI fail, so we should override the automerge. lithium_rebase
to fail during the conda build workflow. lithium_rebase
branch into main
of RMG-database. echem-rebase
back to using main
branch of RMG-database. This will not fail since the database branch was already merged. echem-rebase
branch into main
of RMG-Py.The echem PR has been pretty mature and stable at this point, we hope we could have it deployed by the end of the week. The list will be updated as we progress through each step.
main
. But this will remain a WIP until we merge #2687 So after you finish this checklist will users be able to use RMG-electrocat? Or will they have to wait for the development version of RMS that you are using to be merged into RMS main?
I will note that the PR you are waiting on (#2687) is currently mostly in progress by me in #2694, and I not able to get the new juliacall
code to work. I have currently made RMS optional (see #2735) and intend to just get the Python 3.9 upgrade to work first, and then go back and add RMS back after.
Perhaps we need to Zoom...
So after you finish this checklist will users be able to use RMG-electrocat? Or will they have to wait for the development version of RMS that you are using to be merged into RMS main?
The idea is that they would be able to use it. Until the next release of course, users would have to be using the "install from source" or developer docker version.
@ssun30 please add to the checklist (and then add a commit to this PR to do it) editing the "install from source" instructions for users so that they use the appropriate marcus_development
branch of RMS. (maybe we should pick a better name for it, @mjohnson541 ? this is essentially a branch of RMS for use by main
RMG)
I am confused, the current installation instructions still say to use the version of julia
we install from conda-forge
, but this update requires using julia
installed via juliaup
(i.e. what we are doing in #2687)?
I think this PR doesn't change how we install Julia. (Just which branch of RMS is used). We deliberately separated out all the Julia issues to avoid PR dependency stalemate.
I've renamed the RMS branch that this follows to be for_rmg
instead of marcus_development
. We used this name previously (Summer 2023) when we temporarily needed to avoid the main
RMS branch. I updated the CI and documentation to match.
I've renamed the RMS branch that this follows to be
for_rmg
instead ofmarcus_development
. We used this name previously (Summer 2023) when we temporarily needed to avoid themain
RMS branch. I updated the CI and documentation to match.
The RMG_DATABASE_BRANCH
environment variable in the CI was set at the wrong location (under the build-osx
job) in commit 564c62c which causes the CI to fail because the build-and-test-linux
job still uses main
. It should be set as a global environment variable and I have amended this commit.
cat: write error: Resource temporarily unavailable WARNING:root:Initial mole fractions do not sum to one; normalizing. cat: write error: Resource temporarily unavailable WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. ⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
cat: write error: Resource temporarily unavailable WARNING:root:Initial mole fractions do not sum to one; normalizing. cat: write error: Resource temporarily unavailable WARNING:root:Initial mole fractions do not sum to one; normalizing. WARNING:root:Initial mole fractions do not sum to one; normalizing. ⚠️ One or more regression tests failed. Please download the failed results and run the tests locally or check the log to see why.
beep boop this comment was written by a bot :robot:
Electrochemistry!
This PR finally brings electrochemistry to RMG. It replaces or combines #2000 and #2316. Most of the original work was done by @davidfarinajr and @mjohnson541. Help and effort rebasing from @JacksonBurns and @ssun30. With some assistance along the way by @rwest.
Over the years it been checked many times. All the tests now pass. It goes with RMG-database pull request https://github.com/ReactionMechanismGenerator/RMG-database/pull/667 and it uses the marcus_development branch of RMS (which it checks out during the CI). The manual installation instructions need tweaking to help people install that branch, or we need to merge that branch to main in RMS.