SunnySuite / Sunny.jl

Spin dynamics and generalization to SU(N) coherent states
Other
86 stars 19 forks source link

Some type-parameter grabbing code is very opaque #127

Closed Lazersmoke closed 2 months ago

Lazersmoke commented 1 year ago

Arcane:

https://github.com/SunnySuite/Sunny.jl/blob/b852192d0044effe21bf66684e57f1879d6d4953/src/Intensities/LinearSpinWaveIntensities.jl#L67C19-L67C62

kbarros commented 1 year ago

Currently we define

struct ClassicalIntensityFormula{T} <: IntensityFormula
    kT :: Float64
    formfactors
    string_formula :: String
    calc_intensity :: Function
end

Note that T is never used in the data, but is rather used to carry type-level information that will be reified later.

In this context:

typeof(formula).parameters[1].parameters[2]

I think @Lazersmoke said that formula::ClassicalIntensityFormula{T}, and T is referring to a ::BandStructure{_, ReturnType}. The purpose above is to reify the ReturnType type parameter into the "value" (runtime data) level.

To avoid these tricks in the type system, I wonder if we can move T from the type level to the data level, i.e.

struct ClassicalIntensityFormula <: IntensityFormula
    type_info :: <<perhaps a tuple of types >>
    kT :: Float64
    formfactors :: Any
    string_formula :: String
    calc_intensity :: Function
end
Lazersmoke commented 1 year ago

Yep I like Kip's proposal to move it to value level. The real issue here is that Julia Function can't be annotated with its return type :(

kbarros commented 1 year ago

For future reference, I thought this wouldn't work, but it does! Julia automatically reifies type-level parameters to data-level as needed (this is different than, say, Java). I still think it's better that we keep the information at the data level explicitly, but this is interesting to know.

struct Foo{ImportantData} end

many_foos = Foo[]
push!(many_foos, Foo{42}(), Foo{43}())

many_foos[1]
many_foos[2]
@assert typeof(many_foos[1]).parameters[1] == 42
@assert typeof(many_foos[2]).parameters[1] == 43
kbarros commented 2 months ago

Resolved in #288.