Bring the "for RMG" developments into main. #270
Conversation
Removed an invalid field in a Marcus type reaction in ORR.rms
`cat` is a default Julia function for concatenating arrays. As a precaution these are renamed to `catalyst` to avoid conflicts
…that are only available locally
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)
Attempting to Fix the `juliacall` Overhaul on RMG
There was a problem hiding this comment.
Pull Request Overview
This PR integrates the “for RMG” developments into main by introducing a new parameter (d) to various domain and rate functions and updating related API calls. Key changes include:
- Modifying domain functions in src/Domain.jl to pass an extra parameter (d) (and additional placeholders such as 0.0) in calls to kinetic functions.
- Updating rate calculation functions in src/Calculators/Ratevec.jl and src/Calculators/Rate.jl to include extra parameters (d and dGrxn) in their inline definitions.
- Replacing PyPlot with PythonPlot in several iJulia notebooks and updating dependency/CI configurations in documentation, environment files, and workflows.
Reviewed Changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Domain.jl | Propagates a new parameter “d” in domain constructors and function calls to kinetic routines. |
| src/Calculators/Ratevec.jl & src/Calculators/Rate.jl | Adds extra parameters (d, dGrxn) to inline rate functions and updates their signatures. |
| iJulia/*.ipynb | Updates plotting package reference from PyPlot to PythonPlot. |
| docs/src/Installation.md, deps/build.jl, .github/workflows/* | Revises dependency management and CI/CD workflows to reflect new conventions. |
Comments suppressed due to low confidence (1)
.github/workflows/CI.yml:16
- [nitpick] The workflow now uses newer versions of GitHub Actions (e.g., checkout@v4). Consider pinning the action versions or using semver ranges to ensure reproducibility across builds.
- uses: actions/checkout@v2
| @@ -85,7 +86,7 @@ function ConstantTPDomain(; phase::E2, initialconds::Dict{X,X2}, constantspecies | |||
| C = P / (R * T) | |||
| V = N * R * T / P | |||
| y0[end] = V | |||
| kfs, krevs = getkfkrevs(phase, T, P, C, N, ns, Gs, diffs, V, 0.0) | |||
| kfs, krevs = getkfkrevs(phase, T, P, C, N, ns, Gs, diffs, V, 0.0, 0.0) | |||
There was a problem hiding this comment.
Ensure that the new 'd' parameter is consistently passed to all calls of getkfkrevs and that the parameter order aligns correctly with the updated function signature to avoid unintended behavior.
Copilot uses AI. Check for mistakes.
| @@ -504,6 +506,7 @@ function ConstantTVDomain(; phase::Z, initialconds::Dict{X,E}, constantspecies:: | |||
| V = 0.0 | |||
| P = 1.0e8 | |||
| phi = 0.0 | |||
| d = 0.0 | |||
There was a problem hiding this comment.
[nitpick] The pattern of adding a new domain parameter (d) is repeated across several functions. Consider refactoring these repetitive patterns into a helper function to improve maintainability and reduce potential errors.
| d = 0.0 | |
| d = initialize_domain_parameter(initialconds, "d", 0.0) |
Copilot uses AI. Check for mistakes.
| @inline (arr::Arrhenius)(;T::Q,P::N=0.0,C::S=0.0,phi=0.0,dGrxn=0.0,d=0.0) where {Q<:Real,N<:Real,S<:Real} = @fastmath arr.A*T^arr.n*exp(-arr.Ea/(R*T)) | ||
| @inline (arr::Arrhenius)(T::Q;P::N=0.0,C::S=0.0,phi=0.0,d=0.0) where {Q<:Real,N<:Real,S<:Real} = @fastmath arr.A*T^arr.n*exp(-arr.Ea/(R*T)) |
There was a problem hiding this comment.
The extra parameters (dGrxn and d) are accepted in the rate function but not used in the rate expression. Verify that this is intentional; if these parameters are reserved for future use, consider documenting their purpose or remove them if unnecessary.
| @inline (arr::Arrhenius)(;T::Q,P::N=0.0,C::S=0.0,phi=0.0,dGrxn=0.0,d=0.0) where {Q<:Real,N<:Real,S<:Real} = @fastmath arr.A*T^arr.n*exp(-arr.Ea/(R*T)) | |
| @inline (arr::Arrhenius)(T::Q;P::N=0.0,C::S=0.0,phi=0.0,d=0.0) where {Q<:Real,N<:Real,S<:Real} = @fastmath arr.A*T^arr.n*exp(-arr.Ea/(R*T)) | |
| @inline (arr::Arrhenius)(;T::Q,P::N=0.0,C::S=0.0,phi=0.0) where {Q<:Real,N<:Real,S<:Real} = @fastmath arr.A*T^arr.n*exp(-arr.Ea/(R*T)) | |
| @inline (arr::Arrhenius)(T::Q;P::N=0.0,C::S=0.0,phi=0.0) where {Q<:Real,N<:Real,S<:Real} = @fastmath arr.A*T^arr.n*exp(-arr.Ea/(R*T)) |
Copilot uses AI. Check for mistakes.
| Pkg.build("PyCall") | ||
| ENV["JULIA_CONDAPKG_BACKEND"] = "Null" | ||
| ENV["JULIA_PYTHONCALL_EXE"] = "/path/to/python" | ||
| Pkg.add("CondaPkg") |
There was a problem hiding this comment.
The installation instructions have been updated to use CondaPkg and PythonCall. Please ensure that the documentation clearly reflects these changes so that users can correctly set up their environment.
Copilot uses AI. Check for mistakes.
Is there any reason for these changes not to be part of RMS main, now that they work?
We shouldn't delete the
for_rmgbranch, but the closer it stays tomainthe better, IMHO.