-
-
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
Chemistry functionality remake #718
Conversation
Ok, this shoudl be ready now. I will have a look tomorrow with fresh eyes though to be sure there are no errors (there are a couple in the docs for a starter that I need to fix). One feature that it might make sense to add is a Finally, I don't think setting metadata is supported for compounds, which probably should be the case, e.g. @compound H2_2(t) [metadata=value] = 2H
@compounds begin
O2 [metadata=value]= 2O
end |
Ok, this should be ready now. |
Yes, we should support metadata for compounds. Should we instead infer the arguments of the compound from the species that make it up? |
I.e. if the species depend on time |
That's a good point. Having a compound with a different independent variable than its compositions would be weird, right? If this is indeed the case I will change it so that the compound takes its components iv (and throw an error if they have different ones. Supporting metadata should be easy, e.g. @compounds begin
O2 [metadata=value] = 2O
end But if we want to also give a default value, how would we phrase that? @compounds begin
O2 [metadata=value] = 2O =1.0
end ? |
Or the compound could be the union of the |
Yeah, we should allow default values. Maybe we should use something else to indicate the formula in terms of species? @compounds begin
O2 [metadata=value] = 1.0 ~ 2O
end But I’m open to other suggestions. |
I will have a look tomorrow at what is actually possible notations using metaprogramming. Leaning towards keeping the current notation, and then add something if default values are required. |
The one reason for |
With default values being stored in the metadata, would it be possible to define it there? Doesn't seem to work perfectly though, e.g. @variables t X(t) [defaultvalue=1.0] and other alternatives I've tried fail. Stuff like this:
would be possible. I think that, for now, I will leave setting default values unsupported (unless we can make it work along the line like |
OK, if it isn't easy to do maybe we should hold off on making an update then. I'd prefer not to potentially break the interface twice (i.e. once with this PR, and then potentially again when we fix it to support metadata and initial values). |
Updated how @compound H2O_1 ~ 2H + O
@compound H2O_2, [output=true] ~ 2H + O
@compound (H2O_3 = 1.5) ~ 2H + O
@compound (H2O_4 = 4, [output=true]) ~ 2H + O
@compound (H2O_5 = p1, [output=true]) ~ 2H + p2*O Currently, I have not been able to figure out how to extract the independent variable from the rhs, so just set it to |
a95e5f4
to
d607736
Compare
Sorry, I've been juggling 10 branches back and forth and I finally messed up. I accidentally rebased this on #713 (I meant to do that for the structural identifiability PR, which needs to be on that PR to be able to handle mm and hill functions). Talked to Avik and there doesn't seem to be a good solution more than waiting with merging this until #713 is merged. |
2082d6b
to
0289166
Compare
f20d62a
to
f624106
Compare
Updates:
|
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.
Does balancing with reactions involving compounds of compounds work / is it tested?
@TorkelE I'm fine with merging once you've addressed these comments -- I don't need to review again. If compounds of compounds don't work in balancing that should be mentioned in the tutorial and docstring though (or made to work).
docs/src/catalyst_functionality/chemistry_related_functionality.md
Outdated
Show resolved
Hide resolved
docs/src/catalyst_functionality/chemistry_related_functionality.md
Outdated
Show resolved
Hide resolved
docs/src/catalyst_functionality/chemistry_related_functionality.md
Outdated
Show resolved
Hide resolved
docs/src/catalyst_functionality/chemistry_related_functionality.md
Outdated
Show resolved
Hide resolved
Compound of compounds does not work. Thought it was mentioned, but will add that in (and in docstring as well). |
Can we add an error message in |
(Unless I just missed that we already give an error there.) |
Checked, no there is not such error. I will add that. |
OK, thanks! Once you've made your updates feel free to merge if tests pass. (They might not until we get the binomial issue in Symbolics fixed.) |
…y.md Co-authored-by: Sam Isaacson <[email protected]>
…y.md Co-authored-by: Sam Isaacson <[email protected]>
…y.md Co-authored-by: Sam Isaacson <[email protected]>
Co-authored-by: Sam Isaacson <[email protected]>
…y.md Co-authored-by: Sam Isaacson <[email protected]>
This is done now. Will give it a day or two to see what happens with the binomial issue before merging though (in case there is some problem with that). |
We need to wait till we can run tests to completion before merging anything. |
yes |
Changes:
@compound
syntax:@compound
to see variables, e.g.compounds
macro, enabling the creation of multiple compounds simultaneously:compounds
option to the DSL, enabling the declaration of compounds within it (the components must be designated as a species, and are not ifered).