refactor: Rework parametric types for more flexibility #807
Conversation
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
Link breakage due to commit 0164606. Might not have caught all of them, I can't get documenter to work offline.
|
It's a draft at the moment because the testsuite is segfaulting for me, so I will need to manually investigate if this breaks other things. |
|
Yep, I get that segfault occasionally too. Can't find out where is t coming from. And to add for some unknown reason now the Linux CI always fail for an obscure reason while installing Julia. |
| length::Int # Byte length of postscript | ||
| mode::Int # 1 = Has header, 2 = Has trailer, 3 = Has both | ||
| comment::Array{Any,1} # Cell array with any comments | ||
| end |
There was a problem hiding this comment.
But this would loose the descriptions of each field
There was a problem hiding this comment.
I don't think it should, from this commit you can run julia --project and in the REPL do a ?GMTgrid. This shows the fields as I have added to the source docstrings, e.g. https://github.com/GenericMappingTools/GMT.jl/pull/807/files/8699804706c9979b804e20ad82be0787f98cd972#diff-f757a5fd59739f474cf66fd576eab2d7099ebc0bd28dda3ad57e92bfa6ca7829R1-R32
I'm pretty sure that documenter shows the same information, and also now they are accessible via REPL help.
OH I think I see what you mean now, we lose the default output of e.g. ?GMTgrid:
No documentation found.
Summary
≡≡≡≡≡≡≡≡≡
mutable struct GMTdataset{T<:Real, N}
Fields
≡≡≡≡≡≡≡≡
data :: Array{T<:Real, N}
ds_bbox :: Vector{Float64}
bbox :: Vector{Float64}
attrib :: Dict{String, String}
colnames :: Vector{String}
text :: Vector{String}
header :: String
comment :: Vector{String}
proj4 :: String
wkt :: String
geom :: Int64
Supertype Hierarchy
≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡≡
GMTdataset{T<:Real, N} <: AbstractArray{T<:Real, N} <: Any
I still think the version with descriptions is more valuable. The other option is to document each field and use DocStringExtensions to automatically merge the field docstrings into the type docstring.
There was a problem hiding this comment.
Having field descriptions on REPL would be nice but no big deal. Before there no description at all of the type.
And I missed to see that you had added docstrings in gmt_main.jl, hence my previous comment.
This comment has been minimized.
This comment has been minimized.
| Base.getindex(D::GMTdataset, inds::Vararg{Int,N}) where N = D.data[inds...] | ||
| Base.setindex!(D::GMTdataset, val, inds::Vararg{Int,N}) where N = D.data[inds...] = val | ||
|
|
||
| #Base.BroadcastStyle(::Type{<:GMTdataset}) = Broadcast.ArrayStyle{GMTdataset}() |
There was a problem hiding this comment.
Why you comment these? Not that I really understand what they do. The docs on AbstractArray interface let a lot to be desired.
There was a problem hiding this comment.
They were causing trouble, I'll have to rework them to fit the new semantics if the parametric type changes are accepted. Since GMTdataset is no longer a subtype of AbstractArray I don't know if we need them anymore, but I'm still investigating.
There was a problem hiding this comment.
Hmm, this is more then I know. Will it not break the AbstractArray interface? And I'm considering trying also the Tables interface (have even a branch with an attempt) and think it requires the the tables to be <: AbstractArray
There was a problem hiding this comment.
There was a problem hiding this comment.
Ah I see, I was just going to patch whatever functions needed the array interface to take GMTdataset{AbstractArray{T,N}} instead of GMTDataset{T,N} but doing the table interface that way won't work then.
We could adopt a similar idea to TypedTables.jl, and possibly have our cake and eat it too. The parametric types would wrap an AbstractArray (the "data") but would themselves also be subtypes of AbstractArray:
https://github.com/JuliaData/TypedTables.jl/blob/main/src/Table.jl#L18
There was a problem hiding this comment.
Ok, a little recap. Other that the transpose example what other situations did you get type restricted failures?
Note that we cannot actually allow transpose objects in GMTgrid because we cannot transfer them to C. But we can catch then inside mat2grid and collect them ... as along as I find out out to recognize a transposed object.
There was a problem hiding this comment.
I will have to think about it again, having the mat2<thing> functions "canonicalize" the data seems weird, maybe it's best for me/others to do that upfront before giving it to GMT.jl. I thought there might be a clever way to do the connversion, but after playing with abstract types some more I don't think that's correct after all.
|
Closing for now, I couldn't figure out a nice way to do it at the moment. I might rebase and try again later. |
First step for #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 documentation1.