search

SciML / Catalyst.jl

Chemical reaction network and systems biology interface for scientific machine learning (SciML). High performance, GPU-parallelized, and O(1) solvers in open source software.
https://docs.sciml.ai/Catalyst/stable/
Other
460 stars 77 forks source link

expanding equations when passed to `@reaction_network` #1041

Closed vyudu closed 3 weeks ago

vyudu commented 4 weeks ago

Closes #1022.

isaacsas commented 4 weeks ago

Can you add some tests in the dsl_options.jl file?

TorkelE commented 4 weeks ago

It seems like this extracts parameters from DSL expressions (checking on my phone, so have not gone through properly)? Previously we decided quite strongly against doing any such extractions, and generally keep it to a minimum with the exception of fir reactions (we could change this of course, but it would be a rather dramatic shift)

vyudu commented 4 weeks ago

Ah, I can revert that. I wanted to make 

rn = @reaction_network
    @equations D(A) ~ hill(A, v, K, n)
end

work without throwing an error that v is undefined, but I see that it could be complicated to allow this inference generally. What was the main reason for not letting this, out of curiosity?

TorkelE commented 3 weeks ago

The reason has to do that it can be ambiguous what is a species, a variable, and a parameter, from just looking at the expression. And if there is some misunderstanding, there could be nasty errors.

Given that equations in ReactionSystems is relatively niche, we figured it would be better to have the user manually declare in these situations.

isaacsas commented 3 weeks ago

@vyudu just as a general comment; it is always good to try to keep a PR focused on one thing. That helps keep it shorter and easier to review. I'd keep this one focused on the external function bug and we can discuss what to do about variable/parameter discovery separately.

vyudu commented 3 weeks ago

@TorkelE Why isn't it default behavior to add_default_diff? Since they are not allowed to use D for their own differentials it should not create any issues right? This would make the expressions in #1044 work I think.

TorkelE commented 3 weeks ago

This does a lot of stuff with how equations in the DSL are handled which I don't think we should touch here (e.g. permits f(d(X)) LHS). We did have some reasons to be restrictive with how we infer variables, but I really don't mind expanding this either. However, I think we should let this just handle the function errors, and then we can discuss separately which additional cases we wish to support and not, and handle those in a separate PR.

isaacsas commented 3 weeks ago

@vyudu if you put "Closes issue#" like I edited above that will actually close the issue upon merging.

TorkelE commented 3 weeks ago

I will have a look later today, there is a thing I want to check on my computer.

TorkelE commented 3 weeks ago

If you ctrl + f search for "### " in the dsl_options.jl test file you will find a section with tests for adding coupled equations to the DSL, I'd move the tests there. Otherwise looks good. (we have been meaning to have a short dev doc page somewhere with stuff like this, without it is is quite hard to know about stuff likes that!)