Skip to content
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

Refactor "srcd/sl.jl" file #985

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Catalyst.jl
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

import Graphs: DiGraph, SimpleGraph, SimpleDiGraph, vertices, edges, add_vertices!, nv, ne
import DataStructures: OrderedDict, OrderedSet
import Parameters: @with_kw_noshow
Expand Down
2 changes: 1 addition & 1 deletion src/chemistry_functionality.jl
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

components = :([]) # Becomes something like :([C, O]).
coefficients = :([]) # Becomes something like :([1, 2]).
for comp in composition
Expand Down
792 changes: 419 additions & 373 deletions src/dsl.jl

Large diffs are not rendered by default.

44 changes: 21 additions & 23 deletions src/expression_utils.jl
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,18 @@

# Function that handles variable interpolation.
function esc_dollars!(ex)
if ex isa Expr
if ex.head == :$
return esc(:($(ex.args[1])))
else
for i in 1:length(ex.args)
ex.args[i] = esc_dollars!(ex.args[i])
end
# If we do not have an expression: recursion has finished and we return the input.
isaacsas marked this conversation as resolved.
Show resolved Hide resolved
(ex isa Expr) || (return ex)

# If we have encountered an interpolation, perform the appropriate modification, else recur.
if ex.head == :$
return esc(:($(ex.args[1])))
else
for i in eachindex(ex.args)
ex.args[i] = esc_dollars!(ex.args[i])
end
end

ex
end

Expand All @@ -22,28 +25,23 @@ end
### Parameters/Species/Variables Symbols Correctness Checking ###

# Throws an error when a forbidden symbol is used.
function forbidden_symbol_check(v)
!isempty(intersect(forbidden_symbols_error, v)) &&
error("The following symbol(s) are used as species or parameters: " *
((map(s -> "'" * string(s) * "', ",
intersect(forbidden_symbols_error, v))...)) *
"this is not permitted.")
nothing
function forbidden_symbol_check(sym)
isempty(intersect(forbidden_symbols_error, sym)) && return
used_forbidden_syms = intersect(forbidden_symbols_error, sym)
Comment on lines +29 to +30
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

error("The following symbol(s) are used as species or parameters: $used_forbidden_syms, this is not permitted.")
end

# Throws an error when a forbidden variable is used (a forbidden symbol that is not `:t`).
function forbidden_variable_check(v)
!isempty(intersect(forbidden_variables_error, v)) &&
error("The following symbol(s) are used as variables: " *
((map(s -> "'" * string(s) * "', ",
intersect(forbidden_variables_error, v))...)) *
"this is not permitted.")
function forbidden_variable_check(sym)
isempty(intersect(forbidden_variables_error, sym)) && return
used_forbidden_syms = intersect(forbidden_variables_error, sym)
Comment on lines +36 to +37
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

error("The following symbol(s) are used as variables: $used_forbidden_syms, this is not permitted.")
end

# Checks that no symbol was sued for multiple purposes.
function unique_symbol_check(syms)
allunique(syms) ||
error("Reaction network independent variables, parameters, species, and variables must all have distinct names, but a duplicate has been detected. ")
nothing
allunique(syms) && return
error("Reaction network independent variables, parameters, species, and variables must all have distinct names, but a duplicate has been detected. ")
Comment on lines +43 to +44
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end

### Catalyst-specific Expressions Manipulation ###
Expand Down
2 changes: 1 addition & 1 deletion src/spatial_reaction_systems/spatial_reactions.jl
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ function make_transport_reaction(rateex, species)
forbidden_symbol_check(union([species], parameters))

# Creates expressions corresponding to actual code from the internal DSL representation.
sexprs = get_sexpr([species], Dict{Symbol, Expr}())
sexprs = get_usexpr([species], Dict{Symbol, Expr}())
pexprs = get_pexpr(parameters, Dict{Symbol, Expr}())
iv = :($(DEFAULT_IV_SYM) = default_t())
trxexpr = :(TransportReaction($rateex, $species))
Expand Down
32 changes: 25 additions & 7 deletions test/dsl/dsl_advanced_model_construction.jl
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end

# Miscellaneous interpolation tests. Unsure what they do here (not related to DSL).
Expand Down Expand Up @@ -211,11 +211,11 @@ let
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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

k, 0 --> X, [md1=1.0, md1=1.0]
end
let
@test_throws Exception @eval @reaction k, 0 --> X, [md1=1.0, md1=2.0]
@test_throws Exception @eval @reaction_network begin
k, 0 --> X, [md1=1.0, md1=1.0]
end
end

# Tests for nested metadata.
Expand Down Expand Up @@ -267,6 +267,24 @@ let
@test isequal(rn1,rn2)
end

# Tests that erroneous metadata declarations yields errors.
let
# Malformed metadata/value separator.
@test_throws Exception @eval @reaction_network begin
d, X --> 0, [misc=>"Metadata should use `=`, not `=>`."]
end

# Malformed lhs
@test_throws Exception @eval @reaction_network begin
d, X --> 0, [misc,description=>"Metadata lhs should be a single symbol."]
end

# Malformed metadata separator.
@test_throws Exception @eval @reaction_network begin
d, X --> 0, [misc=>:misc; description="description"]
end
end

### Other Tests ###

# Test floating point stoichiometry work.
Expand Down Expand Up @@ -344,4 +362,4 @@ let
rx = Reaction(k[1]*a+k[2], [X[1], X[2]], [Y, C], [1, V[1]], [V[2] * W, B])
@named arrtest = ReactionSystem([rx], t)
arrtest == rn
end
end
96 changes: 95 additions & 1 deletion test/dsl/dsl_basic_model_construction.jl
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,65 @@ end

## Run Tests ###

# Tests the various network constructors. Test for `@network_component` and `@network_component`.
# Tests for combinations of reactions/no reactions, no name/name/interpolated name.
let
# Declare comparison networks programmatically.
@parameters d
@species X(t)
rx = Reaction(d, [X], [])

rs_empty = ReactionSystem([], t; name = :name)
rs = ReactionSystem([rx], t; name = :name)
rs_empty_comp = complete(rs_empty)
rs_comp = complete(rs)

# Declare empty networks.
name_sym = :name
rs_empty_1 = @network_component
rs_empty_2 = @network_component name
rs_empty_3 = @network_component $name_sym
rs_empty_comp_1 = @reaction_network
rs_empty_comp_2 = @reaction_network name
rs_empty_comp_3 = @reaction_network $name_sym

# Check that empty networks are correct.
isequivalent(rs_empty_1, rs_empty)
rs_empty_2 == rs_empty
rs_empty_3 == rs_empty
isequivalent(rs_empty_comp_1, rs_empty_comp)
rs_empty_comp_2 == rs_empty_comp
rs_empty_comp_3 == rs_empty_comp

# Declare non-empty networks.
rs_1 = @network_component begin
d, X --> 0
end
rs_2 = @network_component name begin
d, X --> 0
end
rs_3 = @network_component $name_sym begin
d, X --> 0
end
rs_comp_1 = @reaction_network begin
d, X --> 0
end
rs_comp_2 = @reaction_network name begin
d, X --> 0
end
rs_comp_3 = @reaction_network $name_sym begin
d, X --> 0
end

# Check that non-empty networks are correct.
isequivalent(rs_1, rs)
rs_2 == rs
rs_3 == rs
isequivalent(rs_empty_1, rs_empty)
rs_empty_2 == rs_empty
rs_empty_3 == rs_empty
end
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# Test basic properties of networks.
let
basic_test(reaction_networks_standard[1], 10, [:X1, :X2, :X3],
Expand Down Expand Up @@ -404,7 +463,7 @@ let
@test rn1 == rn2
end

# Tests arrow variants in `@reaction`` macro .
# Tests arrow variants in `@reaction`` macro.
let
@test isequal((@reaction k, 0 --> X), (@reaction k, X <-- 0))
@test isequal((@reaction k, 0 --> X), (@reaction k, X ⟻ 0))
Expand Down Expand Up @@ -437,6 +496,41 @@ let
@test_throws LoadError @eval @reaction nothing, 0 --> B
@test_throws LoadError @eval @reaction k, 0 --> im
@test_throws LoadError @eval @reaction k, 0 --> nothing

# Checks that non-supported arrow type usage yields error.
@test_throws Exception @eval @reaction_network begin
d, X ⇻ 0
end
end

### Error Test ###

# Erroneous `@reaction` usage.
let
# Bi-directional reaction using the `@reaction` macro.
@test_throws Exception @eval @reaction (k1,k2), X1 <--> X2

# Bundles reactions.
@test_throws Exception @eval @reaction k, (X1,X2) --> 0
end

# Tests that malformed reactions yields errors.
let
# Checks that malformed combinations of entries yields errors.
@test_throws Exception @eval @reaction_network begin
d, X --> 0, Y --> 0
end
@test_throws Exception @eval @reaction_network begin
d, X --> 0, [misc="Ok metadata"], [description="Metadata in (erroneously) extra []."]
end

# Checks that incorrect bundling yields error.
@test_throws Exception @eval @reaction_network begin
(k1,k2,k3), (X1,X2) --> 0
end

# Checks that incorrect stoichiometric expression yields an error.
@test_throws Exception @eval @reaction_network begin
k, X^Y --> XY
end
end
62 changes: 57 additions & 5 deletions test/dsl/dsl_options.jl
Original file line number Diff line number Diff line change
Expand Up @@ -414,15 +414,44 @@ let
spcs = (A, B1, B2, C)
@test issetequal(unknowns(rn), sts)
@test issetequal(species(rn), spcs)
end

@test_throws ArgumentError begin
rn = @reaction_network begin
@variables K
k, K*A --> B
end
# Tests errors in `@variables` declarations.
let
# Variable used as species in reaction.
@test_throws Exception @eval rn = @reaction_network begin
@variables K(t)
k, K + A --> B
end

# Tests error when disallowed name is used for variable.
@test_throws Exception @eval @reaction_network begin
@variables π(t)
end
end


# Tests that duplicate iv/parameter/species/variable names cannot be provided.
let
@test_throws Exception @eval @reaction_network begin
@spatial_ivs X
@species X(t)
end
@test_throws Exception @eval @reaction_network begin
@parameters X
@species X(t)
end
@test_throws Exception @eval @reaction_network begin
@species X(t)
@variables X(t)
end
@test_throws Exception @eval @reaction_network begin
@parameters X
@variables X(t)
end
end


### Test Independent Variable Designations ###

# Test ivs in DSL.
Expand Down Expand Up @@ -739,6 +768,14 @@ let
@observables $X ~ X1 + X2
(k1,k2), X1 <--> X2
end

# Observable metadata provided twice.
@test_throws Exception @eval @reaction_network begin
@species X2 [description="Twice the amount of X"]
@observables (X2, [description="X times two."]) ~ 2X
d, X --> 0
end

end


Expand Down Expand Up @@ -916,8 +953,15 @@ let
@equations X ~ p - S
(P,D), 0 <--> S
end

# Differential equation using a forbidden variable (in the DSL).
@test_throws Exception @eval @reaction_network begin
@equations D(π) ~ -1
end
end

### Other DSL Option Tests ###

# test combinatoric_ratelaws DSL option
let
rn = @reaction_network begin
Expand Down Expand Up @@ -951,3 +995,11 @@ let
@unpack k1, A = rn3
@test isequal(rl, k1*A^2)
end

# Erroneous `@default_noise_scaling` declaration (other noise scaling tests are mostly in the SDE file).
let
# Default noise scaling with multiple entries.
@test_throws Exception @eval @reaction_network begin
@default_noise_scaling η1 η2
end
end
Loading