Skip to content

Attempting to Fix the juliacall Overhaul on RMG #268

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

Merged

Conversation

JacksonBurns
Copy link
Contributor

The branch from which I am making this PR contains some outdated edits from @hwpang which were intended to make the change from pyjulia to juliacall possible. Since there are now edits on this branch of RMS (which is used during the RMG installation process) I am opening this PR to check which (if any) are still required, and to just see what the actual changes are.

@JacksonBurns
Copy link
Contributor Author

The related RMG-Py PR is here: ReactionMechanismGenerator/RMG-Py#2749

this causes an error when the conda backend is set to Null (i.e. julia is disallowed from installing conda packages), see: ReactionMechanismGenerator/RMG-Py#2749 (comment)
@JacksonBurns JacksonBurns marked this pull request as ready for review May 23, 2025 02:29
@JacksonBurns JacksonBurns requested a review from jonwzheng May 23, 2025 02:30
Copy link

@jonwzheng jonwzheng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good. Do you know what's going on with the CI?

@@ -54,14 +54,20 @@ function Reactor(domain::T, y0::Array{T1,1}, tspan::Tuple, interfaces::Z=[]; p::
prectmp = ilu(W, τ=tau)
preccache = Ref(prectmp)

if sparsity > 0.8

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why were these needed in this update?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was done by @hwpang here: e53ade9 but the commit message doesn't mention why - I imagine it's just an empirical observation of what improves the performance.

@JacksonBurns
Copy link
Contributor Author

looks good. Do you know what's going on with the CI?

It's failing on all PRs right now: https://github.com/ReactionMechanismGenerator/ReactionMechanismSimulator.jl/actions/runs/14458289456/job/40545991323?pr=269

@JacksonBurns
Copy link
Contributor Author

@jonwzheng I've tried changing the CI s.t. RMS will figure out Conda on its own (as it should) rather than us trying to set up conda for it in the CI - will have to see if it works.

Copy link
Member

@rwest rwest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a Julia expert, but the commits look reasonable, and it seems to work.

@JacksonBurns JacksonBurns merged commit 43018b1 into ReactionMechanismGenerator:for_rmg May 29, 2025
0 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants