cesaraustralia / Dispersal.jl

Tools for simulating organism dispersal
Other
21 stars 3 forks source link

rethink nsteps for clarity and type stability #89

Closed rafaqz closed 3 years ago

rafaqz commented 3 years ago

Description

This PR makes some small changes to growth nsteps precalculation. The main reason was that it was not type-stable in Float32 because there was no way of setting the type of nsteps. Now instead of being able to set nsteps at all, (which doesn't make sense, it's precalculated for every frame)) you can use the keyword nsteps_type to set the type to Float32 if you need the performance.

Also renamed the precalc_timestep method to precalc_nsteps, as nsteps is precalculated, and the timestep is fixed.

It shouldn't be breaking syntax as no constructors should have been setting nsteps before anyway, as it did nothing.

Changes:

Checklist:

For bugfixes

For new Rules

virgile-baudrot commented 3 years ago

Ok. I'm going to change nstep accordingly in my PR.

The link between type of element within grid and type of nstep wasn't obvious. I understand the multiplication of Float32 with Float64 would result in Float64 so you have to make sure nstep is also Float32.

Would it be simpler for the user to set something like: eltype(grid)(rule.nstep)?

Here are the check I've done to understand:

julia> a = Float32(4)
4.0f0
julia> b = Float64(4)
4.0
julia> typeof(a*b)
Float64
julia> typeof(a*eltype(a)(b))
Float32
rafaqz commented 3 years ago

@virgile-baudrot exactly - you would end up with Float64.

The problem is its calculated in precalcrule for multiple rules using the same method, and we don't really know what grid it will be used with at that point.

I was trying to avoid every rule that uses nsteps having to do that eltype(a)(rule.nsteps) conversion. But maybe that method is actually cleaner as the responsibility is on the package, not the user.

rafaqz commented 3 years ago

This actually can't work, because the grids can be other types:

eltype(grid)(rule.nstep)

So I think I'm back to the original idea now.