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

Symbolics.jl support? #142

Open
LebedevRI opened this issue Apr 23, 2023 · 7 comments
Open

Symbolics.jl support? #142

LebedevRI opened this issue Apr 23, 2023 · 7 comments

Comments

@LebedevRI
Copy link

Currently, Measurement is a subtype of, and operates on, AbstractFloat,
while Symbolics.jl's variables/expressions are subtype of Real.
Given that AbstractFloat is a subtype of Real, currently one can not pass
said symbolic variables through Measurements.jl.

A very rough hack (s/AbstractFloat/Real/) shows that in principle, it works:

(@v1.8) pkg> dev /repositories/Measurements.jl
   Resolving package versions...
  No Changes to `~/.julia/environments/v1.8/Project.toml`
  No Changes to `~/.julia/environments/v1.8/Manifest.toml`

julia> using Symbolics, Measurements
[ Info: Precompiling Measurements [eff96d63-e80a-5855-80a2-b1b0885c5ab7]
WARNING: Method definition measurement(Real) in module Measurements at /repositories/Measurements.jl/src/Measurements.jl:94 overwritten at /repositories/Measurements.jl/src/Measurements.jl:95.
  ** incremental compilation may be fatally broken for this module **


julia> @variables a b c d
4-element Vector{Num}:
 a
 b
 c
 d

julia> Q = measurement(a, b) + measurement(c, d)
Error showing value of type Measurement{Num}:
ERROR: StackOverflowError:

julia> show(stdout, MIME("text/latex"), Q.val)
$$ \begin{equation}
a + c
\end{equation}
 $$
julia> show(stdout, MIME("text/latex"), Q.err)
$$ \begin{equation}
\sqrt{\left|b\right|^{2} + \left|d\right|^{2}}
\end{equation}
 $$
julia> 

Question: conceptually, is this something that could be supported?

@giordano
Copy link
Member

Making Measurement <: AbstractFloat instead of <: Real was a decision taken very early on in #1 (as you can tell from the very low number). I'm not going to reverse that decision.

@LebedevRI
Copy link
Author

Thank you for replying!

Yes, i have seen that issue when looking at git blame for some of the affected lines of code.

I understand that during the computation, the result inevitably ends up being non-integral,
so excluding Integer types and promoting to AbstractFloat upfront makes sense.
I guess, if someone really wants to avoid FP precision issues, they'd use BigFloat's?

However, it is not so clear-cut given symbolic inputs, because there the result will still be symbolic.

Note that i'm not arguing for allowing everything that is a Real!
Perhaps, could something like this be done instead?

  1. Make Measurement{T <: Real} <: Real
  2. Prohibit T that is neither AbstractFloat nor Symbolics::Num (i.e. as compared to now, allow Symbolics::Num)
  3. At contruction time, promote Real's to AbstractFloat's if they are not Symbolics::Num.

@LebedevRI
Copy link
Author

... does that make sense?

@giordano
Copy link
Member

giordano commented May 6, 2023

As I said, I'm not very happy about point 1. Subtypying AbstractFloat instead of Real has several advantages, you get many more methods for free with that instead of <: Real, and point 2 would require having this package depend on Symbolics (assuming that the constraint you want to put would be placed in an inner constructor of the Measurement type), which is also not something I'm very happy about.

@LebedevRI
Copy link
Author

LebedevRI commented May 6, 2023

Thank you for taking a look!

As I said, I'm not very happy about point 1. Subtypying AbstractFloat instead of Real has several advantages, you get many more methods for free with that instead of <: Real

To be honest, i'm not as familiar with julia, so i'm not fully //sure// doing so is necessary,
i'm only guessing that it is. If it works with just Measurement{T <: Real} <: AbstractFloat,
with Measurement{Num}, then great, no need for Measurement{T <: Real} <: Real...

and point 2 would require having this package depend on Symbolics (assuming that the constraint you want to put would be placed in an inner constructor of the Measurement type), which is also not something I'm very happy about.

Yes indeed, i have thought about that issue, and i fully agree with that pain point,
although i was waiting for you to also point it out before filing JuliaSymbolics/Symbolics.jl#898

@LebedevRI
Copy link
Author

Rough proof of concept in #144, with all tests passing, but insufficient test coverage.

As I said, I'm not very happy about point 1. Subtypying AbstractFloat instead of Real has several advantages, you get many more methods for free with that instead of <: Real

To be honest, i'm not as familiar with julia, so i'm not fully //sure// doing so is necessary, i'm only guessing that it is. If it works with just Measurement{T <: Real} <: AbstractFloat, with Measurement{Num}, then great, no need for Measurement{T <: Real} <: Real...

So far it appears that Measurement{T<:Real} <: AbstractFloat is sufficient.

and point 2 would require having this package depend on Symbolics (assuming that the constraint you want to put would be placed in an inner constructor of the Measurement type), which is also not something I'm very happy about.

Yes indeed, i have thought about that issue, and i fully agree with that pain point, although i was waiting for you to also point it out before filing JuliaSymbolics/Symbolics.jl#898

It turns out, we can't depend on Symbolics in the first place, because that creates a dependency cycle.
SciML/RecursiveArrayTools.jl#265 should deal with it.

@LebedevRI
Copy link
Author

LebedevRI commented May 15, 2023

@giordano ok, i have a question.
From the library's perspective, which one of two possible compromises would be preferred:
1. Separating the functions to support symbolic-typed measurements into separate source files, and only loading them when the Symbolics package is also present, via @require in function __init__().
2. or, depending on //some//, hopefully tiny, package, and being able to have all the functions altogether.

I would guess 2. would result in best code, but will require cooperation from either SciML/RecursiveArrayTools.jl#264 or (preferrably )JuliaSymbolics/Symbolics.jl#898.

Edit: found a way to mix both of these in a nice way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants