Sneaky wl-matching bug when using custom illuminants with extended wl range in vismodel()

[thomased]

Just lost a few hours to this one after some inexplicable results kept cropping up. I haven't spend much time looking for the underlying cause yet, but it looks like some kind of wl-matching issue when a 'long' illuminant is fed into vismodel(). The illuminant should just be trimmed to the shortest relevant range (e.g. matching that of the stimulus & vissyst), but it's not (or at least not done correctly), and weird results ensue.

# Create an irradiance spectrum 300-850 nm
irrad <- procspec(as.rspec(data.frame(wl = 300:850, irrad = 1:551)), opt = 'smooth')

# And a trimmed 300-700nm version of the same
irrad_trim <- as.rspec(irrad, lim = c(300, 700))

# Model
data(flowers)
vis.bee <- vismodel(flowers[, 1:2], illum = irrad, visual = "apis", relative = TRUE)
vis.bee.trim <- vismodel(flowers[, 1:2], illum = irrad_trim, visual = "apis", relative = TRUE)

# Output should be identical, but it's not & can produce some way-off results depending on the illuminant
vis.bee
vis.bee.trim

> vis.bee
                               s         m         l lum
Goodenia_heterophylla 0.02707991 0.4637796 0.5091405  NA
> vis.bee.trim
                               s         m         l lum
Goodenia_heterophylla 0.06022257 0.3943517 0.5454257  NA

[thomased]

Oh okay. The check we have:

https://github.com/rmaia/pavo/blob/73a30c2d546cd7523972cc28264a7ff3ff385859/R/vismodel.R#L260-L268

Doesn't look at the wl range of illum or trans (partly because we don't require wl data for them). So when you get to

https://github.com/rmaia/pavo/blob/73a30c2d546cd7523972cc28264a7ff3ff385859/R/vismodel.R#L360

and

https://github.com/rmaia/pavo/blob/73a30c2d546cd7523972cc28264a7ff3ff385859/R/vismodel.R#L371-L374

it's just using the full range of provided data.

[Bisaloo]

Best option seems to be introducing a breaking change and require a wl column in illum.

Best I can think of without a breaking: make sure all objects used in vismodel() have the same length but that doesn't guarantee at all that wl are matching.

[thomased]

Yeah those were my two thoughts. Either require a wl column and add it to the early check, or add a simple length check in (in prepare_userdefined()?) with a message that we're only taking the first x values if the lengths don't match.

[thomased]

Requiring a wl for illum and trans is safest by far, and probably should have been that way from the start (it pre-dates me).

Seems sensible.

[Bisaloo]

How should we go with the breaking change? First add a warning in the next version and then require the wl column?

[thomased]

Yeah that makes sense to me.