Refactor "srcd/sl.jl" file #985

TorkelE · 2024-07-13T22:49:31Z

Also does the "src/expression_utils.jl" file, as that primarily support the dsl file (the other relevant file is the chemistry one).

Primarily four types of changes:

Added comment to code which did not have (primarily older code).
Rewrite some old code (a lot of stuff in the dsl file is stuff I wrote over 6 years ago, which simply can be rewritten better now).
Do some organisations for where the options are handled in the main function. Been meaning to do this pretty much since I introduced them, and now when we have all *for the foreseeable future) it seemed like a good time to do it. Now it should also be easier to see where to add new options in the future.
Rewrite so that the @reaction_network and @network_component macros reuse minimal amount of code (where as previously it the second was basically ctrl+copied versions of the first.

I also add some tests of all the various ways to create models via the DSL, mostly stuff that was not tested before.

TorkelE · 2024-07-13T22:50:11Z

src/Catalyst.jl

@@ -40,6 +40,7 @@ import ModelingToolkit: check_variables,

 import Base: (==), hash, size, getindex, setindex, isless, Sort.defalg, length, show
 import MacroTools, Graphs
+using MacroTools: striplines


this function is used frequenty, and even more frequently when debugging code. Figured it made sense to import directly.

TorkelE · 2024-07-13T22:51:45Z

src/chemistry_functionality.jl

@@ -86,7 +86,7 @@ function make_compound(expr)
    # Cannot extract directly using e.g. "getfield.(composition, :reactant)" because then 
    # we get something like :([:C, :O]), rather than :([C, O]).
    composition = Catalyst.recursive_find_reactants!(expr.args[3], 1,
-        Vector{ReactantStruct}(undef, 0))
+        Vector{ReactantInternal}(undef, 0))


ReactantInternal and ReactionInternal felt like way better name. Especially now with the Reaction struct, having a struct called ReactionStruct seemed like it could confuse new users. Since it is used internally only (during the creation of models in the DSL), I figured the name could reflect that.

How about DSLReactant and DSLReaction? That makes clear their intended use case.

src/expression_utils.jl

TorkelE · 2024-07-13T22:54:17Z

src/expression_utils.jl

+    if !isempty(intersect(forbidden_symbols_error, sym))
+        used_forbidden_syms = intersect(forbidden_symbols_error, sym)
+        error("The following symbol(s) are used as species or parameters: $used_forbidden_syms, this is not permitted.")
+    end


Just rewritten nicer than what I could do things before (e.g. before I knew about interpolating into strings). No need to have unnecessarily convoluted code.

src/dsl.jl

TorkelE · 2024-07-13T23:34:45Z

src/dsl.jl

    variables = vcat(variables_declared, vars_extracted)
-
-    # handle independent variables


Part of the organisations so that options are handled together (in two sets, some before and some after the reactions are considered, depending on whichever is possible for which option)

TorkelE · 2024-07-13T23:35:34Z

src/dsl.jl

    quote
-        $ps
+        $psexpr


just renamed so that everything which is an expression for the code that generates something ends with expr (and not just some of it)

We should put this after the declaration of ivs as time-dependent parameters are now a thing in MTK that we should ultimately support (if it doesn't already work for us).

Do you want to just include the changes of #1024 by hand here? Then we can close that. Probably easiest way to handle this.

src/dsl.jl

TorkelE · 2024-07-13T23:39:44Z

src/dsl.jl

        symvec = gensym()
-        ex = quote
-            $symvec = $ex
+        expr = quote


in some places we used ex and some expr, here I just made it uniform

TorkelE · 2024-07-13T23:41:46Z

test/dsl/dsl_basic_model_construction.jl

+    isequivalent(rs_empty_1, rs_empty)
+    rs_empty_2 == rs_empty
+    rs_empty_3 == rs_empty
+end


WHen I remade how the macor inputs are handled at the top level (nothing, name, reactions, name + reactions) I wanted to add some tests that all of this works for all cases and both macros.

TorkelE · 2024-07-14T00:04:23Z

test/dsl/dsl_advanced_model_construction.jl

@@ -212,8 +212,8 @@ end

 # Checks that repeated metadata throws errors.
 let 
-    @test_throws LoadError @eval @reaction k, 0 --> X, [md1=1.0, md1=2.0] 
-    @test_throws LoadError @eval @reaction_network begin


This was never supposed to be a LoadError in the first place. There was a variable in the error statement that was undeclared, so that yielded an LoadError, which was not the true error.

TorkelE · 2024-07-14T02:11:47Z

test/dsl/dsl_advanced_model_construction.jl

@@ -131,7 +131,7 @@ let
    end
    # Line number nodes aren't ignored so have to be manually removed
    Base.remove_linenums!(ex)
-    @test eval(Catalyst.make_reaction_system(ex)) isa ReactionSystem
+    @test eval(Catalyst.make_reaction_system(ex, QuoteNode(:name))) isa ReactionSystem


adapted to the new make_reaction_system function (which won't generate names for the user)

isaacsas

For future reference, please try not to reorder functions while also modifying them. It would be a lot easier to look at the diff if you had first rewritten them in this PR, and then in a followup PR reordered.

(Obviously for a change of like 50 lines this can be ignored, but when there are hundreds of lines of diff showing it makes the review a lot longer when things get moved around too.)

isaacsas · 2024-08-14T15:17:25Z

src/chemistry_functionality.jl

@@ -86,7 +86,7 @@ function make_compound(expr)
    # Cannot extract directly using e.g. "getfield.(composition, :reactant)" because then 
    # we get something like :([:C, :O]), rather than :([C, O]).
    composition = Catalyst.recursive_find_reactants!(expr.args[3], 1,
-        Vector{ReactantStruct}(undef, 0))
+        Vector{ReactantInternal}(undef, 0))


How about DSLReactant and DSLReaction? That makes clear their intended use case.

isaacsas · 2024-08-14T15:19:58Z

src/expression_utils.jl

+    isempty(intersect(forbidden_symbols_error, sym)) && return
+    used_forbidden_syms = intersect(forbidden_symbols_error, sym)


Suggested change

isempty(intersect(forbidden_symbols_error, sym)) && return

used_forbidden_syms = intersect(forbidden_symbols_error, sym)

used_forbidden_syms = intersect(forbidden_symbols_error, sym)

isempty(used_forbidden_syms) && return

isaacsas · 2024-08-14T15:22:58Z

src/expression_utils.jl

+    isempty(intersect(forbidden_variables_error, sym)) && return
+    used_forbidden_syms = intersect(forbidden_variables_error, sym)


Suggested change

isempty(intersect(forbidden_variables_error, sym)) && return

used_forbidden_syms = intersect(forbidden_variables_error, sym)

used_forbidden_syms = intersect(forbidden_variables_error, sym)

isempty(used_forbidden_syms) && return

isaacsas · 2024-08-14T15:29:11Z

src/expression_utils.jl

+    allunique(syms) && return
+    error("Reaction network independent variables, parameters, species, and variables must all have distinct names, but a duplicate has been detected. ")


If you want to do this here that is fine, but generally let's try to avoid rewriting code that uses || in this way if this is simply a style preference of yours.

isaacsas · 2024-08-14T17:38:54Z

src/dsl.jl

    quote
-        $ps
+        $psexpr


We should put this after the declaration of ivs as time-dependent parameters are now a thing in MTK that we should ultimately support (if it doesn't already work for us).

isaacsas · 2024-08-15T16:35:03Z

src/dsl.jl

+            if !isempty(ivs)
                error("An observable ($obs_name) was given independent variable(s). These should not be given, as they are inferred automatically.")
-            isnothing(defaults) ||
+            end
+            if !isnothing(defaults)
                error("An observable ($obs_name) was given a default value. This is forbidden.")
-            (obs_name in forbidden_symbols_error) &&
+            end
+            if in(obs_name, forbidden_symbols_error)
                error("A forbidden symbol ($(obs_eq.args[2])) was used as an observable name.")


See comments above about dropping && and ||.

isaacsas · 2024-08-15T16:35:47Z

src/dsl.jl

            if (obs_name in species_n_vars_declared) && is_escaped_expr(obs_eq.args[2])
+                println("HERE")


Suggested change

println("HERE")

isaacsas · 2024-08-15T16:39:36Z

src/dsl.jl

-                push!(observed_vars.args,
+                ivs_get_expr = :(unique(reduce(vcat,
+                    [arguments(MT.unwrap(dep)) for dep in $dep_var_expr])))
+                sort_func(iv) = findfirst(MT.getname(iv) == ivs for ivs in ivs_sorted)


Suggested change

sort_func(iv) = findfirst(MT.getname(iv) == ivs for ivs in ivs_sorted)

sort_func(iv) = findfirst(==(MT.getname(iv)), ivs_sorted)

isaacsas · 2024-08-15T16:43:41Z

src/dsl.jl

+macro (but permiting only a single reaction). A more detailed introduction to the syntax can
+be found in the description of `@reaction_network`.


Reference documentation for more detailed description instead.

More generally, I'd suggest following the approach I mentioned for @reaction_network aiming this as a reference on options for users who are looking things up, and referring users to the docs for tutorials / introductions. (Along with keeping some example usage.)

isaacsas · 2024-08-15T16:45:30Z

src/dsl.jl

        $pexprs
        $iv


Suggested change

$pexprs

$iv

$iv

$pexprs

per the other PR we should probably set the iv first.

TorkelE added 10 commits May 27, 2024 19:26

save progress

efd44be

save progress

d2985c0

save progress

b06068f

save progress

5cc8f41

save progress

15273f1

add additional tests

b105b3e

up

1324428

fix

71c9b5c

Merge branch 'master' into dsl_file_improvements

9ab9290

merge fixes

d45f0a7