-
Notifications
You must be signed in to change notification settings - Fork 12
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
Remove parameter T from most all parametric types #33
Comments
It's necessary in the definitions of the concrete types, but you're right we should be able to get rid of it in the abstract types. Can't remember why I put them in tbh, I think I'd just learnt about parametric types and just put then everywhere 😆 |
@jondea good to see you here! Yes, we both just added Even in concrete types I don't see the need to use struct Sphere{T,Dim} <: Shape{T,Dim}
origin::SVector{Dim,T}
radius::T
end but I think we should instead use struct Sphere{Dim} <: Shape{Dim}
origin::AbstractVector{<:AbstractFloat}
radius::AbstractFloat
end It is important to keep track of |
If you use abstract types to define members of a struct then the types will be unknown at compile time. So the members are essentially pointers and a type (also known as boxed). This means that the probably won't be stored contiguously and functions on them will have to dynamically dispatch. If used on all low level types, you will probably see a 2-50x slowdown. I think there's a lot of lower hanging fruit, where types can be removed or hidden. I'll have a stab at this and make a PR. |
Ah yes I remember now, coming back to me like a bad dream. Yes please do have a try at this, you are better qualified than me! |
The PR I've just made should do the bulk of this, although there's a few more things that could be done. For example
|
Also removed the need to use SVector in tests
The type
T
, used to indicate the type of numbers used, i.e. Float64, FLoat32, etc.. appear in many parametric types:and many more... Keeping track of
T
is pointless, as we do not dispatch based on this in any circumstance. It only currently helps enforce that a set of types all use the sameT
. Enforcing this has almost no purpose, and is at times annoying.This
T
should be removed from almost all parametric types.The text was updated successfully, but these errors were encountered: