AlgebraicJulia / DiagrammaticEquations.jl

MIT License
8 stars 1 forks source link

Bad typing for Decapodes #46

Closed GeorgeR227 closed 2 months ago

GeorgeR227 commented 2 months ago

This bug seems to have been introduced in version 0.1.5 and is causing the Decapodes documentation to break. Below is the error caused.

Error: failed to run `@example` block in src/ice_dynamics/ice_dynamics.md:200-203
│ ```@example DEC
│ sim = eval(gensim(ice_dynamics1D, dimension=1))
│ fₘ = sim(sd, generate)
│ ```
│   exception =
│    ArgumentError: cannot parse "dynamics_•2" as Float64

I've confirmed this is due to 0.1.5 since I've bumped the version back down to 0.1.4 and the same page built fine. This will be resolved once the issue is found and tests are added to prevent this issue again.

lukem12345 commented 2 months ago

I suspect it is because types are inferred in order of precedence as given in the type inference rules list. This has come up before. Adding a new operator (i.e. avg_01) to the list changes that order.

lukem12345 commented 2 months ago

So, a rule which infers literals would be a likely cause.

lukem12345 commented 2 months ago

Due to the 2 out of 3 logic for matching, lines like this one can cause trouble:

  (proj1_type = :Literal, proj2_type = :Form1, res_type = :Form1, op_names = [:/, :./, :*, :.*, :^, :.^]),
lukem12345 commented 2 months ago

So, a rule which infers literals would be a likely cause.

Due to the 2 out of 3 logic for matching, lines like this one can cause trouble:

  (proj1_type = :Literal, proj2_type = :Form1, res_type = :Form1, op_names = [:/, :./, :*, :.*, :^, :.^]),

This line that just checks whether 2 out of 3 are :infer should be amended. Literals have to have special treatment. https://github.com/AlgebraicJulia/DiagrammaticEquations.jl/blob/main/src%2Facset.jl#L412

GeorgeR227 commented 2 months ago

Yeah, looking at the Decapode itself, the dynamics_•2 in question is the result of an avg_01, which is then the source of a *. I'm seeing now this issue didn't actually appear in this version but was uncovered instead.

image

This voting logic seems to be coming back to bite us again and again.

lukem12345 commented 2 months ago

Bug not introduced in 0.1.5 AFAICT.

lukem12345 commented 2 months ago

Yeah. One option is to silo rules for Literals into a new list, and perform a different check for those.

GeorgeR227 commented 2 months ago

The inferred version.

image

I'm going to make and post a MWE based off this.