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

unique() drops units #277

Closed
ekatko1 opened this issue Mar 16, 2021 · 2 comments · Fixed by #283
Closed

unique() drops units #277

ekatko1 opened this issue Mar 16, 2021 · 2 comments · Fixed by #283

Comments

@ekatko1
Copy link

ekatko1 commented Mar 16, 2021

set_units(5, g) %>% unique
returns 5
would expect 5 g

Thanks :)

lewinfox added a commit to lewinfox/units that referenced this issue Jun 4, 2021
* Implement `unique.units()`
* Implement `unique.mixed_units()`
* Add tests
@lewinfox
Copy link
Contributor

lewinfox commented Jun 4, 2021

I've had a go at implementing unique.units() and unique.mixed_units(): see lewinfox/units/tree/unique-units.

After changes:

a <- set_units(c(1, 1, 2, 3), kg)
a
#> Units: [kg]
#> [1] 1 1 2 3

unique(a)
#> Units: [kg]
#> [1] 1 2 3

b <- c(set_units(c(1, 1, 2), kg), set_units(c(3, 3, 4), s), allow_mixed = TRUE)
b
#> Mixed units: kg (3), s (3) 
#> 1 [kg], 1 [kg], 2 [kg], 3 [s], 3 [s], 4 [s] 

unique(b)
#> Mixed units: kg (2), s (2) 
#> 1 [kg], 2 [kg], 3 [s], 4 [s]

Before I raise a PR I'd like some feedback on something that took me by surprise. If the mixed_units object is constructed with multiple "blocks" of the same unit then some of the ordering is preserved in the output of unique().

# The object has two "blocks" of `kg`
z <- c(set_units(c(1, 2), kg), set_units(c(3, 4), s), set_units(c(2, 3), kg), allow_mixed = TRUE)
z
#> Mixed units: kg (4), s (2) 
#> 1 [kg], 2 [kg], 3 [s], 4 [s], 2 [kg], 3 [kg] 

My current implementation gives

unique(z)
#> Mixed units: kg (3), s (2) 
#> 1 [kg], 2 [kg], 3 [s], 4 [s], 3 [kg] 

i.e. the result is not ordered by unit (the last 3 [kg] is off on its own). Is it desirable to have like units grouped together?

# Is this better?
unique(z)
#> Mixed units: kg (3), s (2) 
#> 1 [kg], 2 [kg], 3 [kg], 3 [s], 4 [s] 

@Enchufa2
Copy link
Member

Enchufa2 commented Jun 7, 2021

Thanks, PR very welcome. :)

  • In unique.units, please use NextMethod and the .as.units helper, as in rep
  • In unique.mixed_units, please use NextMethod too, and there is a .as.mixed_units helper.

Before I raise a PR I'd like some feedback on something that took me by surprise. If the mixed_units object is constructed with multiple "blocks" of the same unit then some of the ordering is preserved in the output of unique().

Your current implementation is correct: order must be preserved.

lewinfox added a commit to lewinfox/units that referenced this issue Jun 8, 2021
* Implement `unique.units()`
* Implement `unique.mixed_units()`
* Add tests
Enchufa2 added a commit that referenced this issue Jun 8, 2021
Implement `unique()` S3 methods (#277)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants