Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wavelength subsetting fails if formula is passed as a variable #212

Closed
GegznaV opened this issue Jul 14, 2020 · 4 comments
Closed

Wavelength subsetting fails if formula is passed as a variable #212

GegznaV opened this issue Jul 14, 2020 · 4 comments
Assignees
Labels
Priority: 1-high/urgent Type: question ❔ Questions for all to consider.
Milestone

Comments

@GegznaV
Copy link
Collaborator

GegznaV commented Jul 14, 2020

When I pass wavelength range as a variable, functions [ and [[ fail. If I pass the formula directly, no issue is present.
Is it a bug or I do something wrong?

suppressPackageStartupMessages(library(hyperSpec))

# Works
flu[, 1:2, min ~ 320]
#> hyperSpec object
#>    6 spectra
#>    2 data columns
#>    0 data points / spectrum
#> wavelength: lambda/nm [numeric]
#> data:  (6 rows x 2 columns)
#>    1. spc: I[fl]/"a.u." [matrix, array0]
#>    2. filename: filename [character] rawdata/flu1.txt rawdata/flu2.txt ... rawdata/flu6.txt

# Fails
wl_range <- min ~ 320
flu[, 1:2, wl_range]
#> Error in Re(from): non-numeric argument to function

Created on 2020-07-14 by the reprex package (v0.3.0)

@GegznaV GegznaV added the Type: bug 💣 something is not working as it should label Jul 14, 2020
@ximeg
Copy link
Collaborator

ximeg commented Jul 14, 2020

I know this behavior does not seem logical and was surprising for me as well, but this is rooted deeply into the strangeness of the R language. min ~ 320 is an expression in R, and not a numeric range or array. If you save an expression into wl_range, it works:

wl_range <- expression(min ~ 320)
flu[,1:2, wl_range]
#> hyperSpec object
#>    6 spectra
#>    2 data columns
#>    0 data points / spectrum
#> wavelength: lambda/nm [numeric]
#> data:  (6 rows x 2 columns)
#>    1. spc: I[fl]/"a.u." [matrix0]
#>    2. filename: filename [character] rawdata/flu1.txt rawdata/flu2.txt ... rawdata/flu6.txt 

typeof(expression(min ~ 320))
#> [1] "expression"

typeof(min ~ 320)
#> [1] "language"

I'm not sure if anything should or can be done about this. I'd prefer to ask @cbeleites , she knows better.

@ximeg ximeg added Type: question ❔ Questions for all to consider. and removed Type: bug 💣 something is not working as it should labels Jul 14, 2020
@cbeleites
Copy link
Owner

The high priority label is only because this has aspects that I think we should consider in the next video call. After that, it may turn out to be low priority or high priority to be done before the 1.0 milestone - depending on our decision.

@cbeleites
Copy link
Owner

Without looking into details, I think what happens is that a formula can have its own environment attached, and thus if the envirnoment is from .GlobalEnv, min and max will evaluate to functions min() and max() (or to the values of variables min and max if defined).

Possible alternatives:

  • as Roman says, explicitly using an expression rather than a formula helps

  • we could (slowly) move to using other names (e.g. low and high as in ChemoSpec) that are less likely to exist in the user's workspace.
    Iff such a variable exists, there will be maybe even more of a surprise, though.

  • Currently, we don't extrapolate outside the existing wavelength range of the hyperSpec object. I.e. for an object with wavelength range 500 : 1500, calling wl2i(object, 499.999) will return NA.
    If we drop this behaviour, we could access lowest and highest possible wavelength with -Inf and +Inf, respectively. This would completely remove the surprises with formulas.

    Since the "no extrapolation" is useful, though, this should at least be switched by a flag/parameter.

    • It may be possible to detect +Inf/-Inf and have the extrapolation behaviour at least for these.

Any change to the user interface here should be decided before Milestone 1.0

@GegznaV
Copy link
Collaborator Author

GegznaV commented Aug 12, 2021

@GegznaV GegznaV closed this as completed Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: 1-high/urgent Type: question ❔ Questions for all to consider.
Projects
None yet
Development

No branches or pull requests

3 participants