-
-
Notifications
You must be signed in to change notification settings - Fork 76
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
[v14 ready] Implement metadata for Reactions
s
#749
Conversation
It probably doesn't make too much of a difference at this level, but I think the continuity of having it match Symbolics and ModelingToolkit's metadata system would make things cleaner. |
At some point that might make sense. Currently, we do not have any metadata to manage though. I'd suggest merging this and #678. Then at a later stage when we know what we actually want from reaction metadata we can consider implementing a similar system. |
I don’t think we should use Dicts though. That seems pretty heavy for when we have systems with thousands of reactions. Looking at that thread I’d say we are in the use case for ImmutableDicts (small numbers of objects being stored). And we should settle the interface now, otherwise it will end up a breaking change in the future. |
Alternatively we could just use a flat vector and linear search given we don’t expect lots of metadata. |
If we go down the MTK route, we are still happy with the other parts, especially how Is there a good resource on the MTK thing? I don't dislike the idea of using that interface, but I don't want to get bogged down and spend the next couple of days looking at a very niche implementation that we still rate not sure will actually matter within the next couple of years (especially since both I and Catalyst have better stuff to concentrate on). Basically, if we want to go down the MTK route I would want to know what we want and how now, rather than learning details over 3 different revisions of a PR. |
Another alternative should be to simply make it a named Tuple? That would allow indexing of the fields, make it immutable, and be good for storage? That should work equivalently and also be relatively light weigth. |
The last line here is what causes the error: @variables t
@parameters k
@species X(t) X2(t)
metadata = [:md_1 => 1.0, :md_2 => false, :md_3 => "Hello world", :md_4 => :sym, :md_5 => X + X2^k -1, :md_6 => (0.1, 2.0)] No idea why, it all works fine locally. |
Ok, this one is ready now. |
f5c9aa8
to
4aafee6
Compare
4aafee6
to
17b6c1f
Compare
Reactions
sReactions
s
17b6c1f
to
11754dc
Compare
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.
@TorkelE can you remind me why this has diverged so far from the MTK metadata approach / interface, i.e. not declaring new metadata as in
Catalyst.jl/src/reactionsystem.jl
Lines 1 to 7 in faa8fc7
# Catalyst specific symbolics to support SBML | |
struct ParameterConstantSpecies end | |
struct VariableBCSpecies end | |
struct VariableSpecies end | |
Symbolics.option_to_metadata_type(::Val{:isconstantspecies}) = ParameterConstantSpecies | |
Symbolics.option_to_metadata_type(::Val{:isbcspecies}) = VariableBCSpecies | |
Symbolics.option_to_metadata_type(::Val{:isspecies}) = VariableSpecies |
src/reactionsystem.jl
Outdated
@@ -98,7 +98,7 @@ Notes: | |||
- The three-argument form assumes all reactant and product stoichiometric coefficients | |||
are one. | |||
""" | |||
struct Reaction{S, T} | |||
struct Reaction{R, S, T} |
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.
Why not just make the metadata values always be of type Any
? I'm not sure I see the advantage of adding the extra parameteric type here.
docs/src/api/catalyst_api.md
Outdated
@@ -178,6 +178,9 @@ substoichmat | |||
prodstoichmat | |||
netstoichmat | |||
reactionrates | |||
get_metadata_dict |
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.
This seems like a different approach than what we use for symbolics. There we define custom get/has functions for each possible metadata type, not generic functions that take a symbol indicating the metadata we want. Why change the interface here?
I had a discussion with Shashi, who suggested this. There are some special reasons why Symbolics have to do it the way they do it. However, they do not apply to Catalyst, hence this interface can be used (which is way simpler). Can write more extensively, or discuss in person, later. |
The reason for Symbolic's interface doesn't really matter here though -- what matters is that now we are using a completely different interface. (I'm not talking about the internal implementation details but the user facing way to add new metadata, query its existence as an option, and get its value when present in a given |
On the airport right now, but will think more on it when I get back. While I see the advantage of having the same interface, this one is just insanely much easier to use. The Symbolics one feels really internal, and is nothing I see anyone using outside of package development. If you want we can simply remove the doc part of this one, and let it be internal for now though. |
Letäs discuss this one in person, I am still a bit uncertain exactly what you envision would be removed/added/external/internal. |
This is now ready @isaacsas |
end | ||
getnoisescaling(rn) | ||
``` | ||
- Changed fields of internal `Reaction` structure. `ReactionSystems`s saved using `serialize` on previous Catalyst versions cannot be loaded using this (or later) versions. |
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.
This is probably not an issue since we are going to only release this as part of V14, which we will / should make clear everywhere is completely breaking in many ways.
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 can just remove this line then?
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 can have it here, but it probably needs to be right upfront in the full v14 release comments and not buried.
@TorkelE once these are addressed we can merge. |
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
The
Reaction
type now have a.metadata
field:I would have used
Base.ImmutableDict
s, but there are problems with regards to these: https://discourse.julialang.org/t/create-immutabledict-with-values-of-differenrt-types/107831The metadata can be accessed via the following functions:
Metadata can either be added as an optional argument when creating
Reactions
s programmatically:Here, the accessors works like this:
Metadata can also be provided via the DSLs:
Finally, instead of using arrows like
=>
,can be used to remove the substrate expression from the rate. Internally,
only_use_rate=true
is carried through as a metadata to record this. However, it is removed at the end.Currently, no additional metadata is actually used. However,
noise_scaling
will be added after this is merged.Also:
Bool
:const ExprValues = Union{Expr, Symbol, Float64, Int, Bool}
. Here,Bool
s can potentially be inside expressions (likeFloat64
). We missed this before and I realised when working with this. Added it now.Reaction
struct, I added another test file for tests related to this struct.@reaction
and@reaction_network
are created using the same internal function.Considerations:
only_use_rate
remain in the metadata post-creation? This could also be entirely turned into a metadata thing. Alternatively, we keep it as currently implemented, where this can be used as an option, but never actually appear in the metadata.noise_scalling =
, since we could then catch this error.