-
-
Notifications
You must be signed in to change notification settings - Fork 78
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
Add API functions for the other kinds of matrixes that a CRN ODE system can be factored into #1134
base: master
Are you sure you want to change the base?
Conversation
@isaacsas i think this is done mod the Documentation build thing |
|
||
!isempty(pmap) && (rates = substitutevals(rn, pmap, parameters(rn), rates)) | ||
rcmap = reactioncomplexmap(rn); nc = length(rcmap); nr = length(rates) | ||
mtype = eltype(rates) <: Symbolics.BasicSymbolic ? Num : eltype(rates) |
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.
Should this be Num
and not the unwrapped type?
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.
I think it has to, doesn't seem like it's actually possible to have zeros in this matrix if the eltype is BasicSymbolic
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.
Can't it by type Any
?
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.
Is that better than being Num
? I assumed more specific was better
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.
We don't want to return mixtures of Nums and non-Nums across different methods, so we should not re-wrap internal symbolics.
rn.Y^2 * rn.Z^2, | ||
rn.X^3] | ||
Φ_2 = Catalyst.massactionvector(rn; combinatoric_ratelaws = false) | ||
@test isequal(Φ_2, ncrvec) |
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.
Should these kind of tests be issetequal
? We don't generally want to make assumptions about ordering.
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.
Thinking about this more I think we have to check the order for the output to the mass action vector, otherwise building the ODEs with something like (YK\Phi) will not yield correct results. In general the set of ODEs has to be in the iteration order of species(rn)
.
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.
@vyudu just change the returning of Num
s, and then if tests pass you are welcome to merge.
|
||
!isempty(pmap) && (rates = substitutevals(rn, pmap, parameters(rn), rates)) | ||
rcmap = reactioncomplexmap(rn); nc = length(rcmap); nr = length(rates) | ||
mtype = eltype(rates) <: Symbolics.BasicSymbolic ? Num : eltype(rates) |
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.
We don't want to return mixtures of Nums and non-Nums across different methods, so we should not re-wrap internal symbolics.
It looks like right now tests are failing? |
Specifically
laplacianmat
,fluxmat
, andmassactionvector
. Sorry about the terrible looking diff, I just split a lot of the more chemical reaction network-related tests (detailed/complex balance, deficiency theorems, concentration robustness) into a new file crn_theory.jl and added the tests for this to the bottom of thenetwork_analysis.jl
. (Realize I should have done the file splitting in a separate PR but was in too deep already). Closes #1132.