JuliaWaveScattering / MultipleScattering.jl

A Julia library for simulating, processing, and plotting multiple scattering of waves.
Other
46 stars 12 forks source link

Avoid strange parametric types #19

Closed arturgower closed 6 years ago

arturgower commented 6 years ago

In FrequencySimulation, the field particles is defined as:

 particles::Vector{Particle{Dim,PP,S,T} where {PP<:PhysicalProperties,S<:Shape}}

but I think this leads to unintended behaviour:

    sound = Acoustic(0.1,0.1 + 0.0im,2)
    particles = [Particle(sound,Circle([x,0.0], .5)) for x =0.:2.:10.]

    #As expected
    Particles = Vector{Particle{D,P,S,T}} where {D,P<:PhysicalProperties,S<:Shape,T<:AbstractFloat}
    typeof(particles) <: Particles #true 

# However, when calling the constructor of `FrequencySimulation` it will perform the following type cast
    ParticleVec = Array{Particle{2,PP,S,Float64} where {PP<:PhysicalProperties,S<:Shape}}
    particles = ParticleVec(particles)

    # Which leads to:
    typeof(particles) <: Particles # False

This in turn makes it difficult to write functions for sim.particles. Instead we should use:

    particles = [Particle(sound,Circle([x,0.0], .5)) for x =0.:1.:2.]
    ParticleVec = Vector{Particle{2,PP,S,Float64}} where {PP<:PhysicalProperties,S<:Shape}
    particles = ParticleVec(particles)
    typeof(particles) <: Particles # true
jondea commented 6 years ago

Your fix breaks the ability to simulate particles with different shapes/physics because the where is outside the Vector, which enforces that the types are all the same. For example, the following code leads to a type error.

using MultipleScattering
circle = Circle((0.0,0.0),1.0)
rect = Rectangle((2.0,2.0),3.0,2.0)
a = Acoustic(1.0,1.0,2)
particles = [Particle(a,circle), Particle(a,rect)]
source = TwoDimAcousticPlanarSource(a,[1.0,0.0],[0.0,0.0])
sim = FrequencySimulation(a,particles,source)
jondea commented 6 years ago

You were right, it was a mess but I think I've fixed it now. Look at runtests.jl:81 onwards to see if those tests cover your examples. You should be able to use the Particles type everywhere now.

arturgower commented 6 years ago

Yes, all was working with your fix. Note I've now changed the order of the parameters to {T,Dim,FieldDim,PhysicalProperty,Shape}, which will simplify some of these issues. commit 09c29729568468b8d1c477ef2d64bb3616129b67