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

Relax type constraints #806

Open
adigitoleo opened this issue Jan 17, 2022 · 4 comments
Open

Relax type constraints #806

adigitoleo opened this issue Jan 17, 2022 · 4 comments

Comments

@adigitoleo
Copy link

adigitoleo commented Jan 17, 2022

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 JuliaLang/julia#43811

Some ideas:

  • Rework parametric type declarations and constructors
  • ::Array -> ::AbstractArray
  • ::Vector -> ::AbstractVector
  • ::Matrix->::AbstractMatrix`
  • ::String -> ::AbstractString (or removed)
  • remove type constraints for solo methods (unless kept for readability)
adigitoleo added a commit to adigitoleo/GMT.jl that referenced this issue Jan 17, 2022
First step for GenericMappingTools#806.

Example use case: `mat2grid(transpose([1 2 3; 4 5 6]))`

Also added docstrings to the types which can later be automatically included
into the documentation[1].

[1]: https://juliadocs.github.io/Documenter.jl/stable/man/syntax/#@docs-block
@joa-quim
Copy link
Member

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
Copy link
Author

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
Copy link
Author

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
Copy link
Member

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.

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