JuliaPhysics / SolidStateDetectors.jl

Solid state detector field and charge drift simulation in Julia
Other
82 stars 32 forks source link

Replace "Missing" as default value in Simulation #157

Closed fhagemann closed 3 years ago

fhagemann commented 3 years ago

The current implementation of Simulation{T} looks like this:

mutable struct Simulation{T <: SSDFloat} <: AbstractSimulation{T}
    detector::Union{SolidStateDetector{T}, Missing}
    q_eff_imp::Union{EffectiveChargeDensity{T}, Missing} # Effective charge coming from the impurites of the semiconductors
    q_eff_fix::Union{EffectiveChargeDensity{T}, Missing} # Fixed charge coming from fixed space charges, e.g. charged up surface layers
    ϵ_r::Union{DielectricDistribution{T}, Missing}
    point_types::Union{PointTypes{T}, Missing}
    electric_potential::Union{ElectricPotential{T}, Missing}
    weighting_potentials::Vector{Any}
    electric_field::Union{ElectricField{T}, Missing}

    charge_drift_model::Union{<:AbstractChargeDriftModel{T}, Missing}

    electron_drift_field::Union{ElectricField{T}, Missing}
    hole_drift_field::Union{ElectricField{T}, Missing}
end

with the default constructor being

function Simulation{T}() where {T <: SSDFloat}
    Simulation{T}(
        SolidStateDetector{T}(),
        missing,
        missing,
        missing,
        missing,
        missing,
        [missing],
        missing,
        ElectricFieldChargeDriftModel{T}(),
        missing,
        missing
    )
end

I would be in favor of replacing missing with something more type-stable. Why? If simulation.electric_potential = missing, then calling plot(simulation.electric_potential) results in an error that missing cannot be converted to Float64 to be plotted rather than an error message that simulation.electric_potential is not yet calculated.

One way of doing this could be introducing new types, e.g. MissingPointTypes{T} <: PointTypes{T} and changing to point_types::PointTypes{T}. What would be your comments/thoughts on this?

oschulz commented 3 years ago

This is essentially a defaulting mechanism, though, right? If the user specifies missing, we replace it with a default anyway along the way, correct?

fhagemann commented 3 years ago

I'm not quite sure I understand what you mean. When the simulation is defined using a config file, the fields of the Simulation struct will be missing until they are calculated.

For example, sim.electric_potential will stay missing until it is explicitly calculated, e.g. through calculate_electric_potential!(sim) or simulate!(sim).

Sometimes, having missing as default value will lead to rather unhelpful error messages:

oschulz commented 3 years ago

Oh, yes, silly me.

oschulz commented 3 years ago

I would be in favor of replacing missing with something more type-stable

If we can predict the exact type that will result from field-calc, etc., we could will it with "empty" fields, etc. That would be nice for type stability, but can we determine all of the types when Simulation is instantiated?

fhagemann commented 3 years ago

I think, except for the ChargeDriftModel, yes. Most of the types depend on the precision type and the coordinate system and will be defined when calling Simulation and parsing the config file.

For the ChargeDriftModel, we can implement a default.

lmh91 commented 3 years ago

Simulation is a mutable struct and was never intended to be type stable as it is only a collecting struct.

Internal functions which need to be fast should only get fields of the simulation and not the simulation itself. We should fix these parts if that is the case somewhere.

I would close this.

fhagemann commented 3 years ago

Even if this is not type stable: should we introduce our own missing or undefined such that we can throw more helpful warnings when something undefined is plotted?

plot(sim.electric_potential) # missing now gives TypeError: non-boolean (Missing) used in boolean context... It would be more helpful to get something likePotential not defined. Please run calculate_electric_potential!.