search

ReactionMechanismGenerator / ReactionMechanismSimulator.jl

The amazing Reaction Mechanism Simulator for simulating large chemical kinetic mechanisms
https://reactionmechanismgenerator.github.io/ReactionMechanismSimulator.jl
MIT License
70 stars 33 forks source link

Overhaul RMG-RMS Python-Julia dependencies #256

Open hwpang opened 3 months ago

hwpang commented 3 months ago

@mjohnson541 pointed out that this will likely break RMG's CI. Could you point out where in RMG CI do we need to change to avoid breaking it? cc @JacksonBurns

hwpang commented 3 months ago

@mjohnson541 Recording our offline discussion here: We should replace pyjulia with JuliaCall in RMG (see here: https://juliapy.github.io/PythonCall.jl/stable/pycall/). It should make the installation process easier. cc @JacksonBurns

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 51.59574% with 91 lines in your changes missing coverage. Please review.

Project coverage is 48.42%. Comparing base (1f37bfd) to head (6ce8bfa).

:exclamation: Current head 6ce8bfa differs from pull request most recent head 6b8f26d

Please upload reports for the commit 6b8f26d to get more accurate results.

Files Patch % Lines
src/PhaseState.jl 61.76% 52 Missing :warning:
src/ReactionMechanismSimulator.jl 0.00% 27 Missing :warning:
src/Reactor.jl 44.44% 10 Missing :warning:
src/Plotting.jl 0.00% 1 Missing :warning:
src/fluxdiagrams.jl 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #256 +/- ## ========================================== - Coverage 48.71% 48.42% -0.30% ========================================== Files 31 31 Lines 8313 8351 +38 ========================================== - Hits 4050 4044 -6 - Misses 4263 4307 +44 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

JacksonBurns commented 3 months ago

@hwpang you can open a pull request against RMG-Py and change this line of the CI file (https://github.com/ReactionMechanismGenerator/RMG-Py/blob/eed950a389738e70894c25e1d8388bb20ef75947/.github/workflows/CI.yml#L155) to install this branch of RMS from your repository. We can then just see the failures live.

hwpang commented 3 months ago

RMS-RMG twin PR: https://github.com/ReactionMechanismGenerator/RMG-Py/pull/2640

mjohnson541 commented 3 months ago

I've done some verification on the juliacall end: 1) The primary trick I used to make pyrms from pyjulia works with juliacall 2) I think we can get around the need for diffeqpy, at least I think I circumvented our original reason for needing it...which should be nice because pyrms will no longer need the full DifferentialEquations.jl installed.

I think in terms of merge order this would look like: merge the RMS PR, merge a pyrms PR, build new binaries for pyrms, and then merge RMG PR.

hwpang commented 3 months ago

@mjohnson541 Can you make the pyrms PR since you've looked into this? Thanks

mjohnson541 commented 3 months ago

@mjohnson541 Can you make the pyrms PR since you've looked into this? Thanks

Yes, I'm waiting until this PR is building properly, right now I wouldn't be able to test the pyrms PR properly.

hwpang commented 2 months ago

@mjohnson541 I addressed your comment and had a question. I will write pretty commit messages after we agree on the changes

hwpang commented 1 month ago

Removing the duplicate export would cause the test to fail. I undo the change.

hwpang commented 1 month ago

@mjohnson541 This is ready for another review / approve. Need to clean up the commits after approval and coordinate to merge at the same time with https://github.com/ReactionMechanismGenerator/RMG-Py/pull/2640.