Improve algorithm and typing of ActorNeutronics #167
Conversation
| @@ -19,16 +19,18 @@ function Zcoord(n::neutron_particle) | |||
| n.z | |||
| end | |||
|
|
|||
| mutable struct ActorNeutronics <: PlasmaAbstractActor | |||
| dd::IMAS.dd | |||
| mutable struct ActorNeutronics{T} <: PlasmaAbstractActor | |||
There was a problem hiding this comment.
@bclyons12 you raise a good point here about the type of IMAS.dd. This is something I had not originally caught!
In addition of having to define the actors constructors as you have done, we should do the same for all functions that use type annotations. That is ~500 hits across 58 files. Doable but still annoying.
I'll merge this, and actually remove the {T} from the constructor (I just don't like to have the source of an actor being different than the others. I'll open a separate issue specifically to discuss this and decide a course of action.
There was a problem hiding this comment.
@orso82 You need to do this for structs, since IMAS.dd is abstract. You don't need to do it for function arguments though. Since IMAS.dd{T} <: IMAS.dd, having ::IMAS.dd will allow the compiler will dispatch on each separately.
There was a problem hiding this comment.
Ah, ok! you are right! Phew 😅
What threw me off is that you added this constructor.
ActorNeutronics(dd::IMAS.dd{T}, par::ParametersActor; kw...) where {T} = ActorNeutronics{T}(dd, par; kw...)
Is this really necessary? shouldn't ActorNeutronics pick up the right {T} already based on the concrete IMAS.dd type that is passed?
There was a problem hiding this comment.
Only if you don't override the default inner constructor. You have here (something I advocate against 😃 ), so it doesn't make the default outer constructor either.
Here's the relevant bit from https://docs.julialang.org/en/v1/manual/constructors/ :
There was a problem hiding this comment.
You actually might need the inner constructor here, since you use it to accept keywords and Julia doesn't multiple dispatch on keywords. If you made that as an outer constructor, it couldn't distinguish between that and the default inner constructor...
There was a problem hiding this comment.
Ah, that's also something I had not caught.
As you noted, all actors that only have dd and par as their structure need to use a inner constructor to accept keywords arguments. For all other actors I have defined an outer constructor.
No description provided.