Merge pull request #129 from Saityi/unary-op-in-dyvars

Fix bug in handling compound dynamic variable expressions

Project.toml

@@ -2,7 +2,7 @@ name = "StockFlow"
 uuid = "58c4a0e8-2944-4d18-9fa2-e17726aee9e5"
 license = "MIT"
 authors = ["Xiaoyan Li <xiaoyan.lyu.li@gmail.com>"]
-version = "0.2.1"
+version = "0.2.2"
 
 [deps]
 AlgebraicRewriting = "725a01d3-f174-5bbd-84e1-b9417bad95d9"

src/Syntax.jl

@@ -373,7 +373,7 @@ None. This mutates the given dyvars vector.
 function parse_dyvar!(dyvars::Vector{Tuple{Symbol,Expr}}, dyvar::Expr)
     push!(dyvars, parse_dyvar(dyvar))
  end
- 
+
  function parse_dyvar(dyvar::Expr)
      @match dyvar begin
          :($dyvar_name = $dyvar_def) =>
@@ -582,7 +582,7 @@ A vector of dynamic variable definitions suitable for input to StockAndFlowF.
 function dyvar_exprs_to_symbolic_repr(dyvars::Vector{Tuple{Symbol,Expr}})
     syms::Vector{Pair{Symbol,DyvarExprT}} = []
     for (dyvar_name, dyvar_definition) in dyvars
-        if is_binop_or_unary(dyvar_definition)
+        if is_simple_dyvar(dyvar_definition)
             @match dyvar_definition begin
                 Expr(:call, op, a) => push!(syms, (dyvar_name => Ref(a => op)))
                 Expr(:call, op, a, b) => begin
@@ -879,34 +879,36 @@ sum_variables(sum_syntax_elements) =
 
 
 """
-    is_binop_or_unary(e :: Expr)
+    is_simple_dyvar(e :: Expr)
 
-Check if a Julia expression is a call of the form `op(a, b)` or `a op b`
+Check if a Julia expression is a call of the form `op(a, b)` or `a op b`, where `a` and `b` are values, not expressions
 
 ### Input
 - `e` -- a Julia expression
 
 ### Output
-A boolean indicating if the given julia expression is a function call of two parameters.
+A boolean indicating if the given julia expression is a function call of non-expression parameter(s).
 
 ### Examples
 ```julia-repl
-julia> is_binop_or_unary(:(f()))
+julia> is_simple_dyvar(:(f()))
 false
-julia> is_binop_or_unary(:(f(a)))
+julia> is_simple_dyvar(:(f(a)))
 true
-julia> is_binop_or_unary(:(f(a, b)))
+julia> is_simple_dyvar(:(f(a, b)))
 true
-julia> is_binop_or_unary(:(a * b))
+julia> is_simple_dyvar(:(a * b))
 true
-julia> is_binop_or_unary(:(f(a, b, c)))
+julia> is_simple_dyvar(:(f(a, b, c)))
+false
+julia> is_simple_dyvar(:f(a + b, c + d))
 false
 ```
 """
-function is_binop_or_unary(e::Expr)
+function is_simple_dyvar(e::Expr)
     @match e begin
-        Expr(:call, f::Symbol, a) => true
-        Expr(:call, f::Symbol, a, b) => true
+        Expr(:call, f::Symbol, a) => !(typeof(a) <: Expr)
+        Expr(:call, f::Symbol, a, b) => !(typeof(a) <: Expr) && !(typeof(b) <: Expr)
         _ => false
     end
 end
@@ -1077,7 +1079,7 @@ We have a few options, on how we want to distribute mappings.  Way it's done her
 
 """
 function infer_particular_link!(sfsrc, sftgt, f1, f2, map1, map2, destination_vector)
-        
+
     hom1′_mappings = f1(sftgt)
     hom2′_mappings = f2(sftgt)
 
@@ -1089,7 +1091,7 @@ function infer_particular_link!(sfsrc, sftgt, f1, f2, map1, map2, destination_ve
 
         linkmap = tgt[(mapped_index1, mapped_index2)]
 
-        
+
 
         destination_vector[i] = linkmap # updated
     end
@@ -1213,7 +1215,7 @@ function apply_flags(key::Symbol, flags::Set{Symbol}, s::Vector{Symbol})::Vector
         matches = collect(substring_matches(s, string(key)))
 
         new_flags = copy(flags) # copy isn't necessary, probably
-        pop!(new_flags, :~) 
+        pop!(new_flags, :~)
 
         return collect(flatmap(x -> apply_flags(x, new_flags, s), matches)) # this is just in case we add additional flags.  As is, the recursion is unnecessary.
     else
@@ -1331,7 +1333,7 @@ function causal_loop_macro(block)
         return error("Unknown block type for causal loop syntax: " * String(kw))
       _ => current_phase(statement)
     end
-    
+
   end
 
   return CausalLoopF(nodes, edges, polarities)
@@ -1364,8 +1366,3 @@ include("syntax/Stratification.jl")
 include("syntax/Rewrite.jl")
 
 end
-
-
-
-
-

test/Syntax.jl

@@ -2,9 +2,9 @@ using Base: is_unary_and_binary_operator
 using Test
 using StockFlow
 using StockFlow.Syntax
-using StockFlow.Syntax: is_binop_or_unary, sum_variables, 
-infix_expression_to_binops, fnone_value_or_vector, 
-extract_function_name_and_args_expr, is_recursive_dyvar, create_foot, 
+using StockFlow.Syntax: is_simple_dyvar, sum_variables,
+infix_expression_to_binops, fnone_value_or_vector,
+extract_function_name_and_args_expr, is_recursive_dyvar, create_foot,
 apply_flags, substitute_symbols, DSLArgument
 
 
@@ -20,15 +20,18 @@ end
     include("syntax/Rewrite.jl")
 end
 
-@testset "is_binop_or_unary recognises binops" begin
-    @test is_binop_or_unary(:(a + b))
-    @test is_binop_or_unary(:(f(a, b)))
-    @test is_binop_or_unary(:(1.0 + x))
+@testset "is_simple_dyvar recognises simple expressions" begin
+    @test is_simple_dyvar(:(exp(a)))
+    @test is_simple_dyvar(:(a + b))
+    @test is_simple_dyvar(:(f(a, b)))
+    @test is_simple_dyvar(:(1.0 + x))
 end
-@testset "is_binop_or_unary recognises non-binops as non-binops" begin
-    @test !is_binop_or_unary(:(f()))
-    @test !is_binop_or_unary(:(a + b + c))
-    @test !is_binop_or_unary(:(f(a, b, c)))
+
+@testset "is_simple_dyvar recognises complex expressions as non-simple" begin
+    @test !is_simple_dyvar(:(exp(a + b + c)))
+    @test !is_simple_dyvar(:(f()))
+    @test !is_simple_dyvar(:(a + b + c))
+    @test !is_simple_dyvar(:(f(a, b, c)))
 end
 
 @testset "sum_variables" begin
@@ -369,7 +372,7 @@ end
 
     # S: 1 -> 1 and SV: 1 -> 1 implies LS: 1 -> 1
     @test (infer_links(
-        (@stock_and_flow begin; :stocks; A; :sums; NA = [A]; end), 
+        (@stock_and_flow begin; :stocks; A; :sums; NA = [A]; end),
         (@stock_and_flow begin; :stocks; B; :sums; NB = [B]; end),
         Dict{Symbol, Vector{Int64}}(:S => [1], :F => [], :SV => [1], :P => [], :V => []))
     == Dict(:LS => [1], :LSV => [], :LV => [], :I => [], :O => [], :LPV => [], :LVV => []))
@@ -378,18 +381,18 @@ end
     # that is, vA = A + A, vA -> vB, A -> implies that the As in the vA definition map to the Bs in the vB definition
     # But both As link to the same stock and dynamic variable so just looking at those isn't enough to figure out what it maps to.
     # There will exist cases where it's impossible to tell - eg, when there exist multiple duplicate links, and some positions don't match up.
-   
+
     # It does not currently look at the operator.  You could therefore map vA = A + A -> vB = B * B
     # I can see this being useful, actually, specifically when mapping between + and -, * and /, etc.  Probably logs and powers too.
     # Just need to be aware that it won't say it's invalid.
     @test (infer_links(
-        (@stock_and_flow begin; :stocks; A; :dynamic_variables; vA = A + A; end), 
+        (@stock_and_flow begin; :stocks; A; :dynamic_variables; vA = A + A; end),
         (@stock_and_flow begin; :stocks; B; :dynamic_variables; vB = B + B; end),
         Dict{Symbol, Vector{Int64}}(:S => [1], :F => [], :SV => [], :P => [], :V => [1]))
     == Dict(:LS => [], :LSV => [], :LV => [2,2], :I => [], :O => [], :LPV => [], :LVV => [])) # If duplicate values, always map to end.
 
     @test (infer_links(
-        (@stock_and_flow begin; :stocks; A; :parameters; pA; :dynamic_variables; vA = A + pA; end), 
+        (@stock_and_flow begin; :stocks; A; :parameters; pA; :dynamic_variables; vA = A + pA; end),
         (@stock_and_flow begin; :stocks; B; :parameters; pB; :dynamic_variables; vB = pB + B; end),
         Dict{Symbol, Vector{Int64}}(:S => [1], :F => [], :SV => [], :P => [1], :V => [1]))
     == Dict(:LS => [], :LSV => [], :LV => [1], :I => [], :O => [], :LPV => [1], :LVV => []))
@@ -435,15 +438,15 @@ end
         Dict{Symbol, Vector{Int64}}(:S => [1,1,1], :F => [1,1], :SV => [1,2,3], :P => [1,1], :V => [1,1]))
     == Dict(:LS => [1,3,1,2,3,1,3], :LSV => [], :LV => [1,1], :I => [1,1], :O => [1,1], :LPV => [1,1], :LVV => []))
 
- 
+
 end
 
 
 @testset "Applying flags can correctly find substring matches" begin
-    @test apply_flags(:f_, Set([:~]), Vector{Symbol}()) == [] 
-    @test apply_flags(:f_, Set([:~]), [:f_death, :f_birth]) == [:f_death, :f_birth] 
-    @test apply_flags(:NOMATCH, Set([:~]), [:f_death, :f_birth]) == [] 
-    @test apply_flags(:f_birth, Set([:~]), [:f_death, :f_birth]) == [:f_birth] 
+    @test apply_flags(:f_, Set([:~]), Vector{Symbol}()) == []
+    @test apply_flags(:f_, Set([:~]), [:f_death, :f_birth]) == [:f_death, :f_birth]
+    @test apply_flags(:NOMATCH, Set([:~]), [:f_death, :f_birth]) == []
+    @test apply_flags(:f_birth, Set([:~]), [:f_death, :f_birth]) == [:f_birth]
     @test apply_flags(:f_birth, Set{Symbol}(), [:f_death, :f_birth]) == [:f_birth]
 
     # Note, apply_flags is specifically meant to work on vectors without duplicates; the vector which is input are the keys of a dictionary.
@@ -502,13 +505,13 @@ end
     (@stock_and_flow begin
         :stocks
         A
-        
+
         :dynamic_variables
         v1 = A + A
-    end), 
+    end),
     Dict{Symbol, Vector{Int64}}(:S => [1], :V => [1,1])))
 
-    
+
 
     # Mapping it all to I
 
@@ -573,7 +576,7 @@ end
     @test (@causal_loop begin end) == CausalLoopF()
     @test (@causal_loop begin; :nodes; A; end) == CausalLoopF([:A], [],[])
     @test (@causal_loop begin; :nodes; A; :edges; A = A; end) == CausalLoopF([:A], [:A => :A], [POL_ZERO])
-    
+
     @test (@causal_loop begin
         :nodes
         A
@@ -585,15 +588,15 @@ end
         A ^ A
         A ~ A
     end) == CausalLoopF([:A], [:A => :A for _ in 1:5], [POL_BALANCING, POL_REINFORCING, POL_ZERO, POL_NOT_WELL_DEFINED, POL_UNKNOWN])
-    
+
     @test (@causal_loop begin
        :nodes
        A
        B
-       
+
        :edges
        A > B
        B < A
     end) == CausalLoopF([:A, :B], [:A => :B, :B => :A], [POL_BALANCING, POL_REINFORCING])
 
-end
+end