Catalyst.jl icon indicating copy to clipboard operation
Catalyst.jl copied to clipboard

[v14 ready] Implement metadata for `Reactions`s

Open TorkelE opened this issue 1 year ago • 12 comments

The Reaction type now have a .metadata field:

    """
    Contain additional data, such whenever the reaction have a specific noise-scaling expression for
    the chemical Langevin equation.
    """
    metadata::Dict{Symbol, R}

I would have used Base.ImmutableDicts, but there are problems with regards to these: https://discourse.julialang.org/t/create-immutabledict-with-values-of-differenrt-types/107831

The metadata can be accessed via the following functions:

get_metadata_dict
has_metadata
get_metadata

Metadata can either be added as an optional argument when creating Reactionss programmatically:

using Catalyst

@variables t
@parameters k
@species X(t) X2(t)

metadata = Dict(:noise_scaling => 0.0)
r = Reaction(k, [X], [X2], [2], [1]; metadata=metadata)

Here, the accessors works like this:

get_metadata_dict(r)
has_metadata(r, :noise_scaling)
get_metadata(r, :noise_scaling)

Metadata can also be provided via the DSLs:

rs = @reaction_network begin
    @parameters η
    k, 2X --> X2, [noise_scaling=η]
    k, 2X --> X2, [some_metadata=1.0, more_metadata="Hello world"]
end

@parameters η
@reaction k, 2X --> X2, [noise_scaling=$η]
@reaction k, 2X --> X2, [some_metadata=1.0, more_metadata="Hello world"]

Finally, instead of using arrows like =>,

rs = @reaction_network begin
    k, 2X --> X2, [only_use_rate=true]
end

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:

  • I have updated this, adding Bool: const ExprValues = Union{Expr, Symbol, Float64, Int, Bool}. Here, Bools can potentially be inside expressions (like Float64). We missed this before and I realised when working with this. Added it now.
  • Since some tests here are specific to the Reaction struct, I added another test file for tests related to this struct.
  • I made a minor internal remake so that reactions created in @reaction and @reaction_network are created using the same internal function.

Considerations:

  • Should 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.
  • Should we maintain a list of permitted metadata? This would prevent the user to add their own, but would give us more control. Alternatively, we can add a warning if another metadata was used. This would be useful if someone e.g. writers noise_scalling = , since we could then catch this error.

TorkelE avatar Dec 20 '23 11:12 TorkelE

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.

ChrisRackauckas avatar Dec 20 '23 11:12 ChrisRackauckas

At some point that might make sense. Currently, we do not have any metadata to manage though. I'd suggest merging this and https://github.com/SciML/Catalyst.jl/pull/678. Then at a later stage when we know what we actually want from reaction metadata we can consider implementing a similar system.

TorkelE avatar Dec 20 '23 12:12 TorkelE

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.

isaacsas avatar Dec 20 '23 12:12 isaacsas

Alternatively we could just use a flat vector and linear search given we don’t expect lots of metadata.

isaacsas avatar Dec 20 '23 12:12 isaacsas

If we go down the MTK route, we are still happy with the other parts, especially how only_use_rate is handled?

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.

TorkelE avatar Dec 20 '23 13:12 TorkelE

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.

TorkelE avatar Dec 20 '23 13:12 TorkelE

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.

TorkelE avatar Dec 21 '23 22:12 TorkelE

Ok, this one is ready now.

TorkelE avatar Dec 22 '23 14:12 TorkelE

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.

TorkelE avatar Feb 28 '24 20:02 TorkelE

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 Reaction.) However we internally implement the details why not make the external interface for declaring a new metadata type, querying existence of a given type within a reaction, and getting values of a specific type of metadata the same?

isaacsas avatar Feb 28 '24 20:02 isaacsas

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.

TorkelE avatar Feb 28 '24 22:02 TorkelE

Letäs discuss this one in person, I am still a bit uncertain exactly what you envision would be removed/added/external/internal.

TorkelE avatar Mar 01 '24 01:03 TorkelE

This is now ready @isaacsas

TorkelE avatar Mar 10 '24 00:03 TorkelE

@TorkelE once these are addressed we can merge.

isaacsas avatar Mar 18 '24 13:03 isaacsas