rmaia / pavo

tools for the analysis of color data in R
http://pavo.colrverse.com
GNU General Public License v2.0
69 stars 17 forks source link

CIE doesn't like non-vismodel data #201

Closed thomased closed 4 years ago

thomased commented 4 years ago

From someone trying to use MICA data:

fakedat <- data.frame(X = c(0.1, 0.2), 
                      Y = c(0.3, 0.4),
                      Z = c(0.5, 0.6),
                      lum = c(NA, NA))

colspace(fakedat, space = "cielab")
Error in h[h < 0] <- h[h < 0] + 360 : 
  NAs are not allowed in subscripted assignments
thomased commented 4 years ago

Ah, right. https://github.com/rmaia/pavo/blob/4903b8b9884c4e573ed8a4cdfa0ab4165dc80e55/R/cie.R#L65-L68

My head hasn't been in CIEspace for a while. I wonder if there's a possible workaround or sensible default we could 'guess' with a message....

thomased commented 4 years ago

One obvious idea would be to add illum and visual argument to cie() for those bringing their own data. Users could specify one of the built-ins if they happen to have used functions that match ours (which is possible for something like cie-10 degree & D65, for example). Or they can drop in the actual curves they used.

Bisaloo commented 4 years ago

Do we know if MICA will always use the same parameters to compute these values (e.g., always D65)? If so, it might be interesting at some point to add a function like import_mica() that sets all the attributes required downstream.

Bisaloo commented 4 years ago

This doesn't solve this issue though since users might import data from other sources :thinking:. I would go with https://github.com/rmaia/pavo/issues/201#issuecomment-641101693 as you suggest.

thomased commented 4 years ago

Yeah, seems like the simplest solution. MICA can in principle use anything for their calculations, like we can. I'm pretty sure we already share the same set of curves for viewers (cie-2 & cie-10) and illuminants (D65, forestshade etc.) though, which should make life easier for users.

Something like import_mica() is an interesting idea more generally though - worth some more thought.