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

keep_units() with non-units x input #392

Open
d-morrison opened this issue Feb 14, 2025 · 8 comments · May be fixed by #394
Open

keep_units() with non-units x input #392

d-morrison opened this issue Feb 14, 2025 · 8 comments · May be fixed by #394

Comments

@d-morrison
Copy link

d-morrison commented Feb 14, 2025

If keep_units() is called with a non-units x value, would it be reasonable to just pass through that value to the normal function and not apply units at the end? For example:

units::keep_units(rexp, n = 100, x = 3)
#> Error in UseMethod("units"): no applicable method for 'units' applied to an object of class "c('double', 'numeric')"

Created on 2025-02-13 with reprex v2.1.1

This would be helpful in a function I'm writing where I don't know in advance if the user will provide an input with units or not. As a workaround I can check for units on the input and only call keep_units() when there are units to keep, but built-in behavior would be great!

@d-morrison
Copy link
Author

PS - I'm now realizing I picked a particularly unfortunate example, because in the exponential distribution, the units of the rate parameter need to be inverted for the output, if they are present, so I'll need more than keep_units() for that particular case anyway. Nevertheless, pass-through behavior for non-units still seems potentially useful?

@Enchufa2
Copy link
Member

Enchufa2 commented Feb 14, 2025

keep_units is a pretty simple convenience wrapper

> units::keep_units
function(FUN, x, ..., unit=units(x)) {
  set_units(do.call(FUN, list(x, ...)), unit, mode="standard")
}
<bytecode: 0x55bcfb8018d0>
<environment: namespace:units>

that, as you can see, calls the function and sets the units of the result equal to the one of the first argument. So I'm not sure how we should handle a general case in which there is an indeterminate number of arguments, and where the user may want to keep the unit of an arbritrary argument, with potentially units of different types in different arguments, and also non-units objects here and there. :) IMHO such special cases should be treated individually.

@d-morrison
Copy link
Author

Thanks for your reply! At the risk of overstaying my welcome, how about something like this:

keep_units <- function(FUN, x, ..., unit=units(x)) {
  if (inherits(x, "units")) {
    set_units(do.call(FUN, list(x, ...)), unit, mode = "standard")
  } else {
    warning("`x` does not have units.") # maybe not necessary to warn?
    do.call(FUN, list(x, ...))
  }
}

It could also throw a warning in the else clause. Still pretty simple, but handles the test case above. Understandable if out of scope; apologies if unhelpful!

@Enchufa2
Copy link
Member

It is simple enough, and I do think that the warning is appropriate. Would you open a PR with this?

@d-morrison
Copy link
Author

Thanks! Will do.

@Enchufa2
Copy link
Member

One question about this use case. Wouldn't you want to do something like this?

lims <- set_units(c(1, 3), "s")
keep_units(runif, 3, lims[1], lims[2], unit=units(lims))
#> Units: [s]
#> [1] 2.738217 2.520332 2.669949

This currently works. But it wouldn't with your approach.

@Enchufa2
Copy link
Member

Or

rate <- set_units(3, "1/min")
keep_units(rexp, 3, rate, unit=units(1/rate))
#> Units: [min]
#> [1] 0.70244335 0.38413054 0.05673904

d-morrison added a commit to d-morrison/units that referenced this issue Feb 14, 2025
@d-morrison
Copy link
Author

d-morrison commented Feb 14, 2025

One question about this use case. Wouldn't you want to do something like this?

lims <- set_units(c(1, 3), "s")
keep_units(runif, 3, lims[1], lims[2], unit=units(lims))
#> Units: [s]
#> [1] 2.738217 2.520332 2.669949
This currently works. But it wouldn't with your approach.

Good point; in my use case, I'm currently getting around the "first-argument defines units" issue by naming the n argument:

lims <- set_units(c(1, 3), "s")
keep_units(runif, n = 3, lims[1], lims[2])

I hadn't noticed the unit argument in keep_units(); I'm thinking about whether I can add a bit more logic to handle that better. I set up a draft PR (#394), but I'm still working on it; I'll convert to full PR asap (might be a few days before I can circle back to it)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants