Define `|>` as an infix operator for `uconvert`

[j-fu]

Hi, great work since the start of the package!

With this PR I propose to provide

1u"m" |> us"nm"

in addition to

1u"m" |> uconvert(us"nm")

Would be just more convenient, but may be I overlook some reason why this is not a good idea.

[github-actions[bot]]

Benchmark Results

	main	230b7b52ef1b6e...	main/230b7b52ef1b6e...
Quantity/creation/Quantity(x)	4.33 ± 0.009 ns	3.41 ± 0.01 ns	1.27
Quantity/creation/Quantity(x, length=y)	3.11 ± 0.01 ns	3.11 ± 0.001 ns	1
Quantity/with_numbers/*real	3.11 ± 0.01 ns	3.1 ± 0.01 ns	1
Quantity/with_numbers/^int	8.05 ± 2.2 ns	8.05 ± 2.2 ns	1
Quantity/with_numbers/^int * real	8.67 ± 2.2 ns	8.67 ± 2.2 ns	1
Quantity/with_quantity/+y	4.04 ± 0.001 ns	4.04 ± 0.001 ns	1
Quantity/with_quantity//y	3.42 ± 0.01 ns	3.42 ± 0.011 ns	1
Quantity/with_self/dimension	3.1 ± 0.01 ns	3.1 ± 0.01 ns	1
Quantity/with_self/inv	3.11 ± 0.001 ns	3.11 ± 0.001 ns	1
Quantity/with_self/ustrip	2.79 ± 0.01 ns	2.79 ± 0.01 ns	1
QuantityArray/broadcasting/multi_array_of_quantities	0.145 ± 0.00048 ms	0.145 ± 0.00071 ms	1
QuantityArray/broadcasting/multi_normal_array	0.0528 ± 0.00023 ms	0.0528 ± 0.00059 ms	1
QuantityArray/broadcasting/multi_quantity_array	0.155 ± 0.00059 ms	0.155 ± 0.0006 ms	0.999
QuantityArray/broadcasting/x^2_array_of_quantities	23.1 ± 2.1 μs	23.1 ± 1.6 μs	1
QuantityArray/broadcasting/x^2_normal_array	4.22 ± 0.88 μs	4.16 ± 1.1 μs	1.01
QuantityArray/broadcasting/x^2_quantity_array	6.93 ± 0.25 μs	6.9 ± 0.24 μs	1
QuantityArray/broadcasting/x^4_array_of_quantities	0.0783 ± 0.00046 ms	0.0785 ± 0.00041 ms	0.998
QuantityArray/broadcasting/x^4_normal_array	0.0467 ± 0.00016 ms	0.0466 ± 0.00013 ms	1
QuantityArray/broadcasting/x^4_quantity_array	0.0529 ± 0.0031 ms	0.0529 ± 0.00017 ms	0.999
time_to_load	0.127 ± 0.00019 s	0.127 ± 0.00011 s	1

Benchmark Plots

A plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR. Go to "Actions"->"Benchmark a pull request"->[the most recent run]->"Artifacts" (at the bottom).

[MilesCranmer]

Interested in hearing thoughts from any or all of @odow @chrisrackauckas @devmotion @mikeingold @jkrumbiegel @gaurav-arya.

For the record, I am 50/50 on this. For example you can find some things that don't conform to this, such as if you try to "call" a regular number:

julia> (2)(3)
6

the syntax proposed here would basically make quantities callable...

[ChrisRackauckas]

Float64(1.0), Duall(1.0), it's very common for the constructor of a number type to be a conversion.

[ChrisRackauckas]

Does this add any invalidations?

[MilesCranmer]

This is adding (::Quantity), not (::Type{Quantity}). So that you can do

1u"m" |> us"nm"

to convert to nm rather than

1u"m" |> uconvert(us"nm")

I am ambivalent.

[mikeingold]

I will add that I've never really thought about the implications of this behavior, but that I regularly use something like it Unitful, e.g.: 1.0*mW/(cm^2*sr*μm) |> W/(m^2*sr*nm). That being said, if I had to wrap the right-hand-side with a uconvert, I don't think I'd object.

[MilesCranmer]

@j-fu I'm on board to add this after playing with it a bit. Also the fact that Unitful does it is very helpful!

Any other comments from people? If there are no complaints I'll move forward with it.

[mikeingold]

I finally just got a chance to check this out in a terminal and now I think I understand the distinction.

Leλ_units_preferred = us"mW/(m^2*μm)"   # should also have sr⁻¹, but not currently supported

1.0us"W/(cm^2*nm)" |> Leλ_units_preferred
# I'm used to using this kind of shorthand in Unitful.jl
# throws a MethodError here (type not callable)

1.0us"W/(cm^2*nm)" |> unconvert(Leλ_units_preferred)
# 9.999999999999998e9 m⁻² μm⁻¹ mW

I'm assuming this change would enable the former case. If there isn't any technical/philosophical reason to disallow this, then I'd see this change as positive. That said, other than for quick informal code, now that I've had to actually think about the mechanics of this, I think I'd tend toward using the more explicit uconvert(Leλ_units_preferred, 1.0us"W/(cm^2*nm)") in more formal code.

[devmotion]

I am not in favour of this definition, because it doesn't seem natural to me that instances (!) of symbolic quantities would be callable (and even if one disregards this, the uconvert definition doesn't seem to be completely obvious to me). I think this sentiment is even stronger when thinking about default function application syntax instead of the special pipe syntax. The benefit of this definition seems very minor to me (saving a few characters?).

[j-fu]

From my experience in unitful, I kind of expected this functionality and wondered if it was left out so far for a reason - that is why I submitted the PR, and up to now there doesn't seem to be a technical obstacle. I guess others coming from unitful would miss this as well.

For me this is a convenient way to present calculation results with the right scale of units. The pattern I have in mind is like

myvoltage=some_calculation()
println("""voltage = $(myvoltage |> us"mV") """)

and I think it doesn't need to be more complicated.

It is implemented as a one-liner, not providing this method here might even trigger people to provide it in their packages...

[devmotion]

I think it's great to have a less verbose alternative to uconvert(us"mV", ...). My feeling is just that the approach in the PR abuses the pipe operator for such a less verbose syntax. While the syntax arguably looks fine when the pipe operator is used, IMO the vanilla syntax us"mV"(myvoltage) is surprising and unintuitive - calling us"mV" seems strange to me. To me it seems cleaner to define a unicode operator as an alias of uconvert, which would allow for similarly compact syntax. Maybe myvoltage \measeq us"mV"?

[MilesCranmer]

It is interesting how intuitive this syntax is. I was trying it out while reviewing it, then switched to another branch, and already had fallen into the habit of |> us"nm" for conversions, and ran into the method error. It is certainly an abuse of syntax, but, purely because of the fact that Unitful.jl does this, I think it does make sense to have here. (The uconvert syntax is also from Unitful)

You can also do a currying approach (already implemented) –

julia> myvoltage |> uconvert(us"mV")

but I agree it's just not as nice. (And also not what Unitful does)

I would be in favor of considering another operator in the future as well. I think for the time being we should merge this because it offers compatibility with Unitful (which has been a priority). And down the road we can introduce an alias and try to emphasize the alias in the docs?

[MilesCranmer]

One compromise is we could potentially look at having the |> method print a warning if used outside of the REPL, and point the user to the more explicit uconvert syntax (similar to Pkg.@pkg_str). That way users would be strongly discouraged from relying on the (::Quantity)(::Quantity) method outside of the REPL?

[mikeingold]

While the syntax arguably looks fine when the pipe operator is used, IMO the vanilla syntax us"mV"(myvoltage) is surprising and unintuitive - calling us"mV" seems strange to me.

Maybe it’s a matter of opinion, but this particular example doesn’t bother me. By contrast, I’d be more concerned about something like

dimval = 2.5us”km”
dimval(7.3us”cm”)

Or the potential for a typo in an expression like 2.5us”m”(3.14us”m”) (missing *)

[MilesCranmer]

This specific example wouldn't be an issue because uconvert only performs the conversion task if ustrip(target) == 1, otherwise it throws an error.

julia> uconvert(1.5us"km", us"nm")
ERROR: AssertionError: You passed a quantity with a non-unit value to uconvert.
Stacktrace:

Which is a tiny bit safer I suppose

[MilesCranmer]

Actually... one other option is to only define this method on |>. Then we don't even need to worry about adversarial examples with function calls.

If you look at what |> actually is, it's just a function in base:

help?> |>
search: |>

  |>(x, f)

  Infix operator which applies function f to the argument x. This allows f(g(x)) to be written x |> g |> f. When
  used with anonymous functions, parentheses are typically required around the definition to get the intended
  chain.

So we can create a new method on it... I think...

---

Edit: yep, we can totally do this. Let's do it this way!

Edit 2: Implemented. Seems to work fine.

[mikeingold]

A custom method for Base.:|>(x::Quantity, u::Quantity) sounds like an interesting option for enabling the pipe syntax without introducing potential corner cases.

For the record, I don’t think the counter-examples listed above are significant enough a concern to stymie adoption of this feature.

[MilesCranmer]

Cool. Let's go with that, it seems like it has all of the positives with none of the negatives.

I pushed this suggestion to @j-fu's branch; hope that's okay. If everyone is happy we can merge this.