Skip to content

Add basetype to strip type parameters from type #46213

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

Closed
wants to merge 1 commit into from

Conversation

GVigne
Copy link

@GVigne GVigne commented Jul 29, 2022

This is a followup of this issue.
Recently I have had the need to get the "base type" of a type, ie to strip type parameters from type. It seems that I am far from being the first to have such a problem, and having to call T.name.wrapper every single time is a bit tedious. Following what I read in the different issues which have been opened, I created the basetype function which does just this.
I just didn't know exactly where to put this script: I saw that typename was already defined in essentials, so that's why I put it there, but maybe it needs to go elsewhere.

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This definition is only partly correct for concrete types, which makes in incorrect in general (#35543 (comment))

@LilithHafner
Copy link
Member

I think these are some examples of what @vtjnash is talking about.

This should work:

basetype(Vector) == Array

And perhaps some of these should also work:

basetype(Union{Vector{Int64}, Vector{Float64}) == Array
basetype(Union{Vector, Matrix}) == Array
basetype(Union{Vector, Matrix, SparseMatrixCSC{Int}}) == Union{Array, SparseMatrixCSC}
basetype(Union{Int, Float64}) == Union{Int, Float64}

Currently, they all throw.

@GVigne
Copy link
Author

GVigne commented Aug 1, 2022

Thanks for the examples! Here is what I came up with to solve them:

basetype(T::Type) = T.name.wrapper
basetype(T::UnionAll) = basetype(T.body)
basetype(x::Union) = Union{basetype(x.a), basetype(x.b)}

Can you think of other examples where this would fail or give unwanted results?

@vtjnash
Copy link
Member

vtjnash commented Aug 1, 2022

The PRs "inspired by" this PR are likely to be wrong, hardcoding incorrect assumptions about the subtyping system into the code, which is why I don't think this PR or the issue that spawned it is a good idea: JuliaArrays/StaticArrays.jl#1064 (comment)

@LilithHafner we already have typename(T) for extracting the TypeName representation from those. That is usually all that you should try to access from it, though even that is quite rare (it shows up in one printing case and in one broadcast API). Perhaps it is not obvious that supertype and Base.typename are the alternate dimensions for examining a type hierarchy, and both are rather poorly documented?

@GVigne
Copy link
Author

GVigne commented Aug 1, 2022

I understand what you are saying. Encouraging package developers to use a fragile function can only lead to bad surprises.
Here is the issue I'm facing and trying to solve with this PR. I can't speak for others, but I had the feeling some people are dealing with the same type of issue. Perhaps it's better for it to stay an unresolved problem for now, but then I think it should be explained somewhere in the docs why this is the case.

I am working on a GPU version of the DFTK package. The goal is to have a unique base of code, and not a duplicate code for GPU. We have identified an array which should always be on GPU if we are doing GPU computations, and so instead of having a generic variable telling us if we are on GPU or CPU, we would like to simply be able to look at the type of this array.
The trick is that all GPU packages are based on GPUArrays.jl, so we would also like to have a code available for NVIDIA GPUs (CUDA.jl) and AMD GPUs (AMDGPU.jl) and so on.
Furthermore, this array stores Static Vectors, and we would like to be able to build arrays of the same "base type" (ie Array, CuArray, ROCArray...) with a different type (Float, Complex...)
supertype doesn't do the trick, as I get for example

DenseArray{SVector{3, Int64}, 3} #I would like Array
GPUArraysCore.AbstractGPUArray{SVector{3, Int64}, 3} #I would like CuArray

Base.typename doesn't do the trick either, as I get a Core.TypeName object. So I have to call typename(typeof(G_vectors)).wrapper, and this comes to hardcoding an assumptions about the subtyping system.
Of course, a valid answer would be "just have a variable encoding this information", but I don't find that very satisfying.

@vtjnash
Copy link
Member

vtjnash commented Aug 1, 2022

we would like to be able to build arrays of the same "base type"

The name of this function is similar and already exists for exactly this purpose and problem

would like to simply be able to look at the type of this array

The name of this function is probably Base.moduleroot(Base.typename(Int).module) === CUDA, though I agree that is rather a mouthful and semi-internal. Or perhaps Base.modulesof!, but that is even more internal and also uninferrable.

@GVigne
Copy link
Author

GVigne commented Aug 1, 2022

I am aware of similar and am currently using it. However, it only works if the way the array is built is GPU-compatible. Sometimes, we just want to build an array on the CPU (this can be "long" as it happens only once) then offload it to the GPU with respect to the end user's hardware. This can happen if the method building the array calls to structures which are not isbits (for example, a structure with has strings or such).

I will investigate on the two functions you gave, but they didn't immediately work so I don't know if they will do the trick.

@GVigne GVigne closed this Aug 2, 2022
@GVigne GVigne reopened this Aug 2, 2022
@GVigne
Copy link
Author

GVigne commented Aug 9, 2022

We figured out a much simpler way to do what we wanted. We simply added the array type in one of our structure as a parametric type, so it's very simple to just get the "base type" of the arrays used for computations.
I understand why you say coding basetype that way is wrong, and I can't see a nice way to code it anyways: since we've got a solution, I think it may be better to just close this issue. Perhaps we could add somewhere in the docs that this is not the correct way to inspect types, and no one should base their code on the subtyping system? This is so future package maintainer don't ask the same question.

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

Successfully merging this pull request may close these issues.

4 participants