Symbolics.jl support

[LebedevRI]

Incomplete, proof of concept, missing tests, etc.

Refs. #142

[codecov]

Codecov Report

Merging #144 (c4029d1) into master (79d4b92) will increase coverage by 0.53%.
Report is 1 commits behind head on master.
The diff coverage is 98.71%.

❗ Current head c4029d1 differs from pull request most recent head 9d565ea. Consider uploading reports for the commit 9d565ea to get more accurate results

@@            Coverage Diff             @@
##           master     #144      +/-   ##
==========================================
+ Coverage   96.60%   97.13%   +0.53%     
==========================================
  Files           9        9              
  Lines         619      629      +10     
==========================================
+ Hits          598      611      +13     
+ Misses         21       18       -3     

Files	Coverage Δ
src/Measurements.jl	88.88% <100.00%> (+13.88%)	⬆️
src/math.jl	97.93% <100.00%> (ø)
src/parsing.jl	100.00% <100.00%> (ø)
src/show.jl	100.00% <100.00%> (ø)
src/utils.jl	100.00% <100.00%> (ø)
src/conversions.jl	92.30% <87.50%> (-3.85%)	⬇️

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

[LebedevRI]

Not really sure about those Julia 1.0 CI failures. I guess they mean
that Symbolics package is not avaliable for such an old Julia version?
I'm not sure how to deal with that.

Ok, i think the change is roughly there.
Any general thoughts on this?
Any general thoughts on the approach with Measurements._is_symbolic()? Is that acceptable?
Any suggestions on desired test coverage?

[longemen3000]

for symbolics in 1.0, i would ditch that. that is, add something like:

@static if Base.VERSION >= v"1.2" #minimum version supported by Symbolics
  @require Symbolics=...
end

But, probably that version doesn't even work like the current Symbolics. the current stable series (5.0.x) supports 1.6 onwards, so i would only test with that, that is:

@static if Base.VERSION >= v"1.6" #minimum version supported by Symbolics 5.x.x
  @require Symbolics=...
end

and add another conditional loading in the symbolic test.

[longemen3000]

on the Measurements._is_symbolic() , maybe something like is_measurement_type(::T) could be used instead?

is_measurement_type(::AbstractFloat) = true
is_measurement_type(::T) = false

[LebedevRI]

on the Measurements._is_symbolic() , maybe something like is_measurement_type(::T) could be used instead?

is_measurement_type(::AbstractFloat) = true
is_measurement_type(::T) = false

As you can see in e.g. function Base.show(io::IO, mtype::mime, measure::Complex{<:Measurement}),
specific code needs to be disabled for symbolic-typed measurements,
and is_measurement_type(::AbstractFloat) = true would interfere with that.

[longemen3000]

You can add another helper function:

show_isissue(i::AbstractFloat)=signbit(i) && !isnan(i)
show_isissue(i::Symbolics.Num) = true

[LebedevRI]

Thank you for taking a look, but unless i'm missing something obvious,
that seems strictly counter-productive, so i'll defer to @giordano on that.

[LebedevRI]

@static if Base.VERSION >= v"1.6" #minimum version supported by Symbolics 5.x.x
  @require Symbolics=...
end

Thank you! That does seem reasonable, though i wonder if Project.toml also is a problem.
Edit: yep, it is a problem.

[LebedevRI]

I've , it is not immediately apparent to me if there's a way to workaround the CI issue of julia-1.0.
As far as i can tell, there is really no way to have a truly conditional optional dependency in Project.toml,
so that would mean Symbolics package would need to not be mentioned in Project.toml at all,
which means that, among other things, it would need to be installed by test itself, which seems hacky?

Is bumping

Measurements.jl/Project.toml

Line 18 in 0b54af0

julia = "1"

up to at least 1.2 (realistically, 1.5 or 1.6) an option?

[giordano]

I haven't had much time to look into it yet, but I have a few quick comments:

• as I had already mentioned before, I'm not much inclined to take on more dependencies, I'd rather reduce them, and support for 1.9 extensions #143 was a move into the right direction
• I think you should be able to extend Measurement{<:T} to T<:Real but make Real <: T <: AbstractFloat error in general and only define a Measurement{Symbolic} method in the Symbolics extension
• I'm ok with requiring Julia v1.6
• to reiterate the first point, I'm not ok with adding more dependencies

[LebedevRI]

Thank you for taking a look!

• as I had already mentioned before, I'm not much inclined to take on more dependencies, I'd rather reduce them, and support for 1.9 extensions #143 was a move into the right direction
• to reiterate the first point, I'm not ok with adding more dependencies

That is exactly what this does, as far as i'm aware, no?

Depending on Symbolics in [extras] is for tests only:
see e.g. https://github.com/JuliaPhysics/Measurements.jl/actions/runs/4996699636/jobs/8950211279?pr=144,
notice how there's no Symbolic in Run julia-actions/julia-buildpkg@latest section,
but there is during Run julia-actions/julia-runtest@latest.

I don't really see how we can do better than that?

• I think you should be able to extend Measurement{<:T} to T<:Real but make Real <: T <: AbstractFloat error in general and only define a Measurement{Symbolic} method in the Symbolics extension

(i'm guessing you meant measurement(val::V[, err::E]) "constructors"?)
I'm not sure i fully follow how to do that, but i agree that the current code is a bit brittle...

• I'm ok with requiring Julia v1.6

Ok, great!

[LebedevRI]

Huh, that's weird, on julia-1.6, Aqua's complaint is non-sensical, because that is exactly how Project.toml already is.

[LebedevRI]

• I think you should be able to extend Measurement{<:T} to T<:Real but make Real <: T <: AbstractFloat error in general and only define a Measurement{Symbolic} method in the Symbolics extension

Hopefully done.

[LebedevRI]

Note that i'm still wondering about the recommended test coverage.
I'm tempted to at least mirror most of the non-symbolic tests,
or go further if they don't cover all the functionality,
but is that what is preferred here?

[LebedevRI]

@giordano hi! has there been any progress with making a new release? :)

[giordano]

Not really, I'm swamped with preparing my JuliaCon talks (still haven't finished some of them.....)

[LebedevRI]

Yay, happy to finally see the release! Rebased.
I don't remember if there was any outstanding requested changes here.

[giordano]

I still need to carefully review this PR, but I'm getting inclined towards accepting the idea.

[LebedevRI]

I still need to carefully review this PR, but I'm getting inclined towards accepting the idea.

Yay :) I'm hopeful the commits are sufficiently atomic to aid in easy review,
but if you want me to somehow further adjust things, just say so.

[LebedevRI]

@giordano question:

Apparently, this is insufficient.
There are cases where the T isn't <: Real,
namely JuMP's variables, the other major case i've wanted to cover:

using JuMP, Measurements
model = Model()
@variable(model, x)
@variable(model, y)
measurement(x, y)

ERROR: MethodError: no method matching measurement(::VariableRef, ::VariableRef)
Stacktrace:
 [1] top-level scope
   @ REPL[5]:1

(VariableRef is <: AbstractJuMPScalar, which is not based on any other type.)

But, the current change as-is is an improvement already.
But i want to cover the other case too, it's kind-of important.

Question: should this patch wait for however months more for the review,
only for the follow-up patch touching rather the same lines to be submitted,
or should i modify this change, thus hopefully minimizing the total diff size?
Or is the latter change (allowing 'opt-in" non-Real T's) a non-starter?

[LebedevRI]

@giordano hi! any thoughts on this / the question in previous comment?

[giordano]

Hey @LebedevRI, I'm genuinely struggling deciding what to do with this PR overall, which is affecting my ability to review the finer details.

I've always been a bit hesitant because there's no way back (short of making a breaking release to recover the desired behaviour) so this has to be designed carefully, I always thought the typing of Measurement was in a sweet spot and touching it with unforeseen consequences made me uncomfortable, and extending Measurements to symbolic computation felt a bit weird for something intrinsically "numerical" (although errors are propagated with a well defined formula). At some point I got convinced this was worth doing, but after you said that this isn't even enough and needs stretching the typing even further (not clear how), now I am very undecided again 😕

[LebedevRI]

@giordano thank you for taking a look!

extending Measurements to symbolic computation felt a bit weird for something intrinsically "numerical" (although errors are propagated with a well defined formula)

That's the thing though, there is absolutely nothing in this library
that strictly requires actually having actual numbers.
There are no funny loops, so [symbolic] function tracing just works.
All that is required is not restricting the T in Measurement{T}.

As an example, distances are intrinsically numerical, aren't they?
Yet Distances.jl does not make any restrictions on the T,
and it happily works with Symbolics/JuMP.

The only times where the numbers are needed in Measurement
is pretty-printing, and eagerly promoting to FP64.

It would be quite unfortunate to not have this functionality,
but it would likewise be rather severely unfortunate
to increase entropy by having that feature elsewhere.

[LebedevRI]

@giordano to perhaps move this forward, could you at least say what, exactly, you suspect
may go wrong if the inner type A of Measurement{A} <: B is no longer constrained?
(not B! it remains AbstractFloat) Any particular snippet?
I'm afraid it's not really possible to address concerns that can't even be formulated.

[giordano]

My main concern is that this would open the door to unpredictable behaviour.

As it is now, all input to Measurement() is converted to an AbstractFloat and then then Measurement <: AbstractFloat, so you're wrapping an AbstractFloat and you're telling the world the wrapper behaves like an AbstractFloat. But if you wrap a type T::Any and then promise it behaves like something else, then you can get into weird situations, i.e. incorrect behaviour, which I'm not really about having to deal with. And since T is unbound, it can go arbitrarily wrong.

[LebedevRI]

This sounds an awful lot like: "[If a tree falls in a forest and no one is around to hear it, does it make a sound?] Well, if we make sure that a tree can not fall because there is no forest, then it certainly does not make a sound." :)

But if you wrap a type T::Any and then promise it behaves like something else, then you can get into weird situations, i.e. incorrect behaviour, which I'm not really about having to deal with. And since T is unbound, it can go arbitrarily wrong.

All of that can totally still happen regardless of whether the T claims to be AbstractFloat-based or not,
as we already see with the current behavior on trying to use Symbolic.jl variable in Measurement:
you get an endless promotion loop.

There is no way in Julia to enforce that some class T actually behaves like an, e.g., AbstractFloat,
there are no contract checks. It would be nice to be able to do X{T} <: T, but that's not possible.

It would be quite unfortunate to not have this functionality,
but it would likewise be rather severely unfortunate
to increase entropy by having that feature elsewhere.

[LebedevRI]

@giordano hi. Have your position changed / do you expect it to change?

[LebedevRI]

@giordano i don't suppose it will change anything if i promise to help deal with whatever issues that will come up with non-Real inputs?

[giordano]

Ok, yes, that'd alleviate alleviate part of my concerns. Let me get back to you next week with other more specific comments.