GenericMappingTools / GMT.jl

Generic Mapping Tools Library Wrapper for Julia
Other
193 stars 28 forks source link

Relax type constraints #806

Open adigitoleo opened 2 years ago

adigitoleo commented 2 years ago

A few times now I have run into MethodErrors from this package due to its strict type constraints. I think some of the type declarations could be relaxed, either by changing to a more abstract type or by removing them if they aren't needed.

See also https://www.oxinabox.net/2020/04/19/Julia-Antipatterns.html#over-constraining-argument-types and https://github.com/JuliaLang/julia/issues/43811

Some ideas:

joa-quim commented 2 years ago

Hi @adigitoleo. We can look at specific cases where I'm being too conservative but I won't introduce abstract types in the GMTdataset because I tried it once and there was a strong penalty in some running times.

Also. I have been introducing more type signatures because when I analyze what JET.jl reports it's an horror of Any propagations. It's incredible but a n = length(X), where X is a Any, n is also a Any even though it can only be a Int. That said, converting String to AbstractStringshould be no problem.

adigitoleo commented 2 years ago

The types will still be concrete, this is just a type parametrisation:

Before change:

julia> ds = GMTdataset([1.0 2.0; 3.0 4.0])
2×2 GMTdataset{Float64, 2}:
 1.0  2.0
 3.0  4.0

julia> isconcretetype(typeof(ds))
true

julia> typeof(ds)
GMTdataset{Float64, 2}

After change:

julia> ds = GMTdataset([1.0 2.0; 3.0 4.0])
2×2 Matrix{Float64}:
 1.0  2.0
 3.0  4.0

julia> isconcretetype(typeof(ds))
true

julia> typeof(ds)
GMTdataset{Matrix{Float64}}

So the semantics changes slightly, the GMT.jl types become containers that wrap a concrete array type, rather than array subtypes:

Before:

julia> GMTdataset <: AbstractArray
true

After:

julia> GMTdataset <: AbstractArray
false

But I don't think there should be a performance impact of using data::T because it's always resolved during construction to a specific concrete type. I could be wrong though, I'll need to do some benchmarking I guess.

adigitoleo commented 2 years ago

Any propagations

I will have a look at JET.jl again, forgot that it existed :) It would be ideal if we could remove the sources of Any rather than blocking propagations with type constraints.

joa-quim commented 2 years ago

A very important source of the Any's is the parsing of d Dict that stores the kwargs arguments. It's a Dict{Symbol, Any} so I don't see what can be done here. This doesn't seem to imply any slowdown in execution but it may in compilation time and latency. I've spend countless hours trying to reduce latency and improve compile times using SnoopCompile but only managed to gain a couple of seconds in compilation.