-
Notifications
You must be signed in to change notification settings - Fork 33
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
Performance Optimizations #31
Conversation
Thanks for inviting me to review this PR! I may need some time to understand some of the changes (Luckily, you have discussed many of them in subgroups), and I will try to finish reviewing ASAP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your great contribution, Matt! I have several comments, please take a look. Some of them could be just me not familiar with Julia or your implementation, thank you in advance for your patience for answering them.
Alow[i] = falloff.arr.A | ||
nlow[i] = falloff.arr.n | ||
Elow[i] = falloff.arr.Ea | ||
Ahigh[i] = 1e20 #very high limited by energy transfer process |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think this may introduce a small difference when comparing the rate we calculate in RMS with the rate from other packages? Do you think it is okay to set it as Inf
to make sure the result is consistent with other packages? I just worried that there may be some case that Pr = klow * C/kinf
is not far less than 1 as expected. So that rate_third_body != klow * C as defined in other packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we take the log10 of Pr so it's not numerically feasible to use Inf. k0 is collision limited and C for which ideal gas assumptions are valid is limited as well so I think this should be fine. I found no discrepencies in tests between running the individual reactions and running them vectorized.
|
||
@inline function getkineticstype(kin::Chebyshev) | ||
return (string(typeof(kin).name),kin.Tmin,kin.Tmax,kin.Pmin,kin.Pmax,size(kin.coefs)) #different opt functions, but always can do | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, should we have exports
for the above functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably won't hurt, added.
typeassert.(thermo,NASA) | ||
return NASAvec([sp.thermo for sp in spcs]) | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export getvecthermo
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
function PdepArrheniusvec(pdeparrs::T) where {T<:AbstractArray} | ||
arrs = Array{Arrhenius,1}() | ||
Ps = pdeparrs[1].Ps | ||
arrs = [Array{Arrhenius,1}() for i = 1:length(Ps)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redundant arrs = Array{Arrhenius,1}()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
src/Phase.jl
Outdated
construct the stochiometric matrix for the reactions and the reaction molecule # change | ||
""" | ||
function getstoichmatrix(spcs,rxns) | ||
M = zeros(length(rxns),length(spcs)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a model generated by RMG with ~50000 reactions and ~1000 reactants, I guess the RAM usage can be around 400 MB. Although it is not quite big since we are using sparse matrix anyway, can we not use zeros(length(rxns), length(spcs)) but simply create three lists and feed them to sparse()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, used spzeros instead of zeros
I think I've resolved all the issues with this. |
Vectorization of kinetics and thermo calculations and general optimizations of rate constant and equilibrium constant computations. In tests this resulted in about a 5.2x speed increase on large ~400 species jobs where the rate coefficients need recomputed each evaluation.