Weird method definition for `>=`

[nsajko]

GMT.jl/src/grd_operations.jl

Lines 216 to 217 in a363699

	# For images
	Base.:>=(I::GMTimage{T}, val::Real) where T <: Unsigned = mat2img(collect(I.image .>= val), I)

The type signature of the method:

• does not make sense semantically

• triggers lots of invalidation, xref precompiling on V.11, V1.12, ... may loose half the precompilation work JuliaLang/julia#59494

Furthermore, I don't think >= is supposed to get methods added to it at all. Adding a method to <= instead might be an improvement.

I suggest not defining that method at all, or at least defining <= instead.

[nsajko]

There's actually lots of weird method additions in that file. Another very bad one is:

GMT.jl/src/grd_operations.jl

Lines 205 to 206 in a363699

	# For images
	Base.:>(I::GMTimage{T}, val::Real) where T <: Unsigned = mat2img(collect(I.image .> val), I)

[joa-quim]

Hi,

First, thanks a lot for looking into this. But I'm a bit lost. I don't get what is so bad in that (and similar) methods. I basically want to be able to do I2 = (I >= 100) and receive back a second image with 0's and 1's (a mask).

Another think is, no matter the number of invalidations raised by those methods, that weights very little in the main problem. If I do not include the grd_operations.jl from the package the difference is very minute.

@time @eval plot(rand(5,2))
  6.340564 seconds (26.37 M allocations: 1.399 GiB, 4.05% gc time, 99.95% compilation time: 100% of which was recompilation)

It was ~6.5 s before.

But off-course I want to understand and fix these invalidations too.

[joa-quim]

If I do comment the definitions of
Base.:>(G1::GMTgrid, val::Number); Base.:>(I::GMTimage{T}, val::Real); function Base.:>=(G1::GMTgrid, val::Number) and Base.:>=(I::GMTimage{T}, val::Real)

then all those invalidation comming from that file go away. But what is the sense in that I can define < and <= versions of those methods but not > and >=?

[nsajko]

• The type signature is nonsensical because GMTimage is an AbstractArray. So you're comparing an array to a scalar.

• Connected to the above, the return type is also wrong, >= should return a Union{Bool, Missing} value.

• But what is the sense in that I can define < and <= versions of those methods but not > and >=?

  Suppose there's a function f and some code is trying to call it. Further suppose the argument types for the call are not concretely known at that point. If the number of possibly matching method is great enough, some compiler optimization become unavailable. For example, if there are no matching methods, the call is sure to throw, even though the argument types are not known exactly. Similarly, if there is exactly one method that may match, then that exact method is sure to get called. If there are exactly two methods, the union-splitting optimization or the world-splitting optimization may be applicable. The more methods there are, basically the compiler is more likely to give up on optimizing in case of the argument types are not statically known exactly at the point of the call.

  These functions, > and >= have generic methods that are both always correct and always performant, so it doesn't make sense to add any more methods to them.

  • The doc string of > already says to add methods to < instead. Just submitted a PR to do the same for >=: doc: >=: say not to implement this function directly JuliaLang/julia#59504

  • Another PR I just submitted that may be worth of note here: delete redundant comparison function methods for BigFloat JuliaLang/julia#59505

[joa-quim]

• The type signature is nonsensical because GMTimage is an AbstractArray. So you're comparing an array to a scalar.

I'm sorry but I am not. The full definition is

Base.:>(I::GMTimage{T}, val::Real) where T <: Unsigned = mat2img(collect(I.image .> val), I)

So I'm doing I.image .> val which is a broadcast over the array.

• Connected to the above, the return type is also wrong, >= should return a Union{Bool, Missing} value.

Maybe, but I don't use Missing's in GMT so a GMTimage will never have a Missing.

But to solve this issue. I don't see how I can define this functionality using < and <=. I already have

Base.:<(I::GMTimage{T}, val::Real) where T <: Unsigned = mat2img(collect(I.image .< val), I)

(to which SnoopCompile has nothing to point out) but I need also the syntax I > 100 to get a mask of the Image (or Grid) under that condition. Or, posed in another way, how can I have the I > 100 syntax without defining a new > method?

[nsajko]

I'm sorry but I am not. The full definition is [...]

That point in my message specifically discusses the type signature of the method, without discussing the method body. Just to point out the various ways in which this is wrong.

Maybe, but I don't use Missing's in GMT so a GMTimage will never have a Missing.

What I'm saying is that these functions should return Bool or Missing, not an array.

[joa-quim]

I'm starting to see your point, but with it I'm left with no option than to remove an existing syntax functionality that is sharply short and clean.

[nsajko]

I don't see how I can define this functionality using < and <=. I already have

Base.:<(I::GMTimage{T}, val::Real) where T <: Unsigned = mat2img(collect(I.image .< val), I)

Missed this question before. PR: #1802.