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

Remove parameter T from most all parametric types #33

Open
arturgower opened this issue Nov 11, 2021 · 5 comments
Open

Remove parameter T from most all parametric types #33

arturgower opened this issue Nov 11, 2021 · 5 comments

Comments

@arturgower
Copy link
Member

The type T, used to indicate the type of numbers used, i.e. Float64, FLoat32, etc.. appear in many parametric types:

Shape{T<:AbstractFloat,Dim}
Particle{T<:AbstractFloat,Dim,P<:PhysicalMedium,S<:Shape}

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 same T. Enforcing this has almost no purpose, and is at times annoying.

This T should be removed from almost all parametric types.

@jondea
Copy link
Member

jondea commented Nov 11, 2021

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 😆

@arturgower
Copy link
Member Author

@jondea good to see you here! Yes, we both just added T everywhere when learning this stuff back in our salad days!

Even in concrete types I don't see the need to use T as a parameter. For example, the shape Sphere is currently defined as

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 Dim for dispatch. But the specific types of radius and origin can be left to the user. If they want efficiency, they can define types which will work better for their case. The way we currently have it, leads to more code being written in development, with no clear advantage.

@jondea
Copy link
Member

jondea commented Nov 12, 2021

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.

@arturgower
Copy link
Member Author

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!

@jondea
Copy link
Member

jondea commented Nov 24, 2021

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

  • Swap {T,Dim} around for concrete types, so that you can write for example, Box{2} rather than Box{T,2} where T. If we are de-emphasising T, it probably makes sense to put it last. It's also reassuring that StaticArrays works like this.
  • There's still a lot of diagonal dispatch that could probably be removed
  • Remove T from result objects and just use whichever type comes out in the wash. This is perhaps a bit controversial, although it would allow you to have truly scalar and vector fields for different physics types.

arturgower added a commit that referenced this issue Dec 8, 2021
Also removed the need to use SVector in tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants