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

piecewise handling fails #112

Closed
paulflang opened this issue Jul 10, 2021 · 18 comments · Fixed by #114
Closed

piecewise handling fails #112

paulflang opened this issue Jul 10, 2021 · 18 comments · Fixed by #114
Labels
bug 🐛 Something isn't working

Comments

@paulflang
Copy link
Collaborator

case 000190 gives me the following error:

ERROR: LoadError: TypeError: non-boolean (Sym{Real, Nothing}) used in boolean context
Stacktrace:
  [1] (::SBML.var"#conv#31"{Dict{String, Any}, SBML.var"#27#32", SBML.var"#28#33"})(x::SBML.MathApply)
    @ SBML C:\Users\wolf5212\.julia\packages\SBML\ISPz3\src\symbolics.jl:106
  [2] convert(::Type{Num}, x::SBML.MathApply; mapping::Dict{String, Any}, convert_time::SBML.var"#27#32", convert_const::SBML.var"#28#33")
    @ SBML C:\Users\wolf5212\.julia\packages\SBML\ISPz3\src\symbolics.jl:112
  [3] convert(::Type{Num}, x::SBML.MathApply)
    @ SBML C:\Users\wolf5212\.julia\packages\SBML\ISPz3\src\symbolics.jl:106
  [4] mtk_reactions(model::SBML.Model)
    @ SBMLToolkit c:\Users\wolf5212\OneDrive - Nexus365\DPhil Project\Programmes\juliadev\dev\SBMLToolkit\src\reactionsystem.jl:97

In #40 we used

function parse_piecewise(val, cond, other)
    IfElse.ifelse(cond, val, other)
end

But could also have to do with handling of Booleans.
cc @anandijain

@exaexa
Copy link
Collaborator

exaexa commented Jul 10, 2021

is there a way to reproduce this? (preferably code)

@paulflang
Copy link
Collaborator Author

sbml_fn = "00190-sbml-l3v2.xml"

ml = readSBML(sbml_fn, doc -> begin
            set_level_and_version(3, 2)(doc)
            convert_simplify_math(doc)
        end)
        
convert(Num, ml.reactions["reaction1"].kinetic_math)

@exaexa
Copy link
Collaborator

exaexa commented Jul 10, 2021

that works for me. What precise model is that?

@exaexa
Copy link
Collaborator

exaexa commented Jul 10, 2021

aaaaaah I didn't see the link. Better always supply the download code or something. :D

@exaexa
Copy link
Collaborator

exaexa commented Jul 10, 2021

So it looks that piecewise can actually accept 1+2k arguments for any int k, and the even ones are the actual boolean conditions. Does that sound right?

@exaexa
Copy link
Collaborator

exaexa commented Jul 10, 2021

@exaexa
Copy link
Collaborator

exaexa commented Jul 10, 2021

OK I fixed the piecewise argument order, now there's a bit more general problem:

using Symbolics
@variables a b
Core.ifelse(a < b, 2, 1)

...gives

ERROR: TypeError: non-boolean (Num) used in boolean context
Stacktrace:
 [1] top-level scope
   @ REPL[3]:1

what's the correct ifelse to use with symbolics that handles this well?

cc @ChrisRackauckas

@ChrisRackauckas
Copy link
Contributor

IfElse.ifelse.

@exaexa exaexa added the bug 🐛 Something isn't working label Jul 10, 2021
@paulflang
Copy link
Collaborator Author

paulflang commented Jul 10, 2021

also, you should return the function without evaluating it. it should only be evaluated when the simulator calls it. in your example neither a nor b has a numerical value, so the first argument to Ifelse.ifelse is of type Num or whatever, but definitely not a boolean.

@exaexa
Copy link
Collaborator

exaexa commented Jul 10, 2021

the eval there is evaluating the function "name", not the actual function!

I see ifelse.ifelse is correctly overloaded, good, I totally missed that fact the last time.

@paulflang
Copy link
Collaborator Author

there was no eval in you code example above.😉

@exaexa
Copy link
Collaborator

exaexa commented Jul 10, 2021

ok then I have no idea what eval you are talking about

@paulflang
Copy link
Collaborator Author

I was not talking about eval. I was just trying to reason why you got

ERROR: TypeError: non-boolean (Num) used in boolean context
Stacktrace:
 [1] top-level scope
   @ REPL[3]:1

@exaexa
Copy link
Collaborator

exaexa commented Jul 10, 2021

anyway I guess this is it? (pls ack in the PR)

julia> convert(Num, SBML.MathApply("piecewise", SBML.Math[SBML.MathIdent("p1"), SBML.MathApply("lt", SBML.Math[SBML.MathIdent("p1"), SBML.MathIdent("p2")]), SBML.MathIdent("p2")]))
IfElse.ifelse(p1 < p2, p1, p2)

Yeah the problem with Num is obvious, I was stuffing Num into the bad ifelse that can't support it. Evaluation order doesn't have much to do with that (it's just extra confusing).

@paulflang
Copy link
Collaborator Author

I don't see where the "lt" comes from. In case 00190 we just have:

        <kineticLaw>
          <math xmlns="http://www.w3.org/1998/Math/MathML">
            <piecewise>
              <piece>
                <ci> p1 </ci>
                <false/>
              </piece>
              <otherwise>
                <ci> p2 </ci>
              </otherwise>
            </piecewise>
          </math>
        </kineticLaw>

@exaexa
Copy link
Collaborator

exaexa commented Jul 10, 2021

nah that was my testcase. 00190 produces p2 for me, which sounds okay (the ifelse gets const-folded down)

@paulflang
Copy link
Collaborator Author

Unfortunately, I found a case where piecewise still fails:

using SBML
using Symbolics

url = "https://raw.githubusercontent.com/sbmlteam/sbml-test-suite/master/cases/semantic/00191/00191-sbml-l3v2.xml"
tmpfile = download(url)
try
    ml = readSBML(tmpfile)
    convert(Num, ml.reactions["reaction1"].kinetic_math)  # works
    convert(Num, ml.reactions["reaction2"].kinetic_math)  # fails
finally
    rm(tmpfile)
end
ERROR: DomainError with �:
Unknown SBML function
Stacktrace:
  [1] allowed_sym
    @ C:\Users\wolf5212\.julia\packages\SBML\XMU1n\src\symbolics.jl:76 [inlined]
  [2] (::SBML.var"#conv#31"{Dict{String, Any}, SBML.var"#27#32", SBML.var"#28#33"})(x::SBML.MathApply)
    @ SBML C:\Users\wolf5212\.julia\packages\SBML\XMU1n\src\symbolics.jl:117
  [3] _broadcast_getindex_evalf
    @ .\broadcast.jl:648 [inlined]
  [4] _broadcast_getindex
    @ .\broadcast.jl:621 [inlined]
  [5] getindex
    @ .\broadcast.jl:575 [inlined]
  [6] copyto_nonleaf!(dest::Vector{SymbolicUtils.Sym{Real, Nothing}}, bc::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Tuple{Base.OneTo{Int64}}, SBML.var"#conv#31"{Dict{String, Any}, SBML.var"#27#32", SBML.var"#28#33"}, Tuple{Base.Broadcast.Extruded{Vector{SBML.Math}, Tuple{Bool}, Tuple{Int64}}}}, 
iter::Base.OneTo{Int64}, state::Int64, count::Int64)
    @ Base.Broadcast .\broadcast.jl:1078
  [7] copy
    @ .\broadcast.jl:930 [inlined]
  [8] materialize(bc::Base.Broadcast.Broadcasted{Base.Broadcast.DefaultArrayStyle{1}, Nothing, SBML.var"#conv#31"{Dict{String, Any}, SBML.var"#27#32", SBML.var"#28#33"}, Tuple{Vector{SBML.Math}}})
    @ Base.Broadcast .\broadcast.jl:883
  [9] (::SBML.var"#conv#31"{Dict{String, Any}, SBML.var"#27#32", SBML.var"#28#33"})(x::SBML.MathApply)
    @ SBML C:\Users\wolf5212\.julia\packages\SBML\XMU1n\src\symbolics.jl:117
 [10] convert(::Type{Num}, x::SBML.MathApply; mapping::Dict{String, Any}, convert_time::SBML.var"#27#32", convert_const::SBML.var"#28#33")
    @ SBML C:\Users\wolf5212\.julia\packages\SBML\XMU1n\src\symbolics.jl:123
 [11] convert(::Type{Num}, x::SBML.MathApply)
    @ SBML C:\Users\wolf5212\.julia\packages\SBML\XMU1n\src\symbolics.jl:117
 [12] top-level scope
    @ REPL[46]:4

@paulflang paulflang reopened this Jul 16, 2021
@exaexa
Copy link
Collaborator

exaexa commented Jul 17, 2021

@paulflang please open new issues for newly found failing test cases.

This doesn't look like piecewise problem, it says "unknown SBML function". The only actual function I see in test 191 is geq aka ≥, is that it? (pls answer in a new issue, we can't keep track of the problems this way)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants