r-quantities / units

Measurement units for R
https://r-quantities.github.io/units
175 stars 28 forks source link

close #89: new install_unit and remove_unit #261

Closed Enchufa2 closed 3 years ago

Enchufa2 commented 3 years ago

Implements @t-kalinowski's idea of leveraging UDUNITS's parsing capabilities to define arbitrary mappings with existing units. The new install_unit deprecates install_symbolic_unit, install_conversion_constant and install_conversion_offset, and remove_unit deprecates remove_symbolic_unit. Some examples:

library(units)

# install a new base unit
install_unit("apple")
# one orange is worth two apples
install_unit("orange", "2 apple")

# a more complicated one
install_unit("strange_unit", "0.1 lg(re m/(5 s)^2) @ 50")

# install a new dimensionless unit
install_unit("person", "unitless")

# several symbols and names can be defined
install_unit("cell")
install_unit(c("CFU", "cfu"), "4 cell", c("colony_forming_unit", "ColonyFormingUnit"))

# remove everything
remove_unit(c("apple", "person", "orange", "strange_unit"))
remove_unit(c("cell", "CFU", "cfu"), c("colony_forming_unit", "ColonyFormingUnit"))
codecov[bot] commented 3 years ago

Codecov Report

Merging #261 (8a5566d) into master (67c608f) will increase coverage by 0.08%. The diff coverage is 96.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #261      +/-   ##
==========================================
+ Coverage   94.27%   94.35%   +0.08%     
==========================================
  Files          18       17       -1     
  Lines         873      850      -23     
==========================================
- Hits          823      802      -21     
+ Misses         50       48       -2     
Impacted Files Coverage Δ
R/user_conversion.R 87.50% <87.50%> (-6.25%) :arrow_down:
R/conversion.R 88.23% <100.00%> (ø)
R/make_units.R 90.59% <100.00%> (ø)
R/options.R 100.00% <100.00%> (ø)
R/udunits.R 100.00% <100.00%> (ø)
src/udunits.cpp 100.00% <100.00%> (+1.62%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 67c608f...8a5566d. Read the comment docs.

Enchufa2 commented 3 years ago

Something fails for i386 on Windows (but not for x86_64). Investigating.

Enchufa2 commented 3 years ago

Issue on Windows solved. Ready for review.

Enchufa2 commented 3 years ago

@edzer Kind reminder. I would also like to have @t-kalinowski's feedback.

t-kalinowski commented 3 years ago

Hi @Enchufa2, I'll be happy to take a closer look this weekend.

t-kalinowski commented 3 years ago

Hi @Enchufa2 ,

This looks like great work. It really simplifies the interface to working with custom units.

Some minor comments: While trying this out, I was surprised to see that some units did not simplify automatically.

#> set_units(1, N*m) 
# 1 [m*N]
#> set_units(1, N.m) 
# 1 [J]

Shouldn't the first example simplify? Also, is the "N.m" syntax new? For some reason I'm not remembering that from when I was working on this package. In any case, I think this is mostly unrelated to your PR here, so perhaps this should be a separate issue.

The only real comment I have is: I think the examples section could benefit from some "real" examples that someone might reasonably encounter; The 1 apple == 2 oranges and person*radian examples confused me more than they clarified how this is meant to be used. Some examples of plausible real-life workflows:

I'm not suggesting all, or even any these examples be added. I list them only to illustrate what I mean when I suggest adding a "real" example. I'm sure there are even better examples of "real" usage that you could come up with from your academic/professional experience.

In any case, this is great work! Really nice to see the progress on this package.

Best, Tomasz

Enchufa2 commented 3 years ago

Thanks, @t-kalinowski, for your feedback.

Shouldn't the first example simplify? Also, is the "N.m" syntax new? For some reason I'm not remembering that from when I was working on this package. In any case, I think this is mostly unrelated to your PR here, so perhaps this should be a separate issue.

The dot has always been supported by UDUNITS-2, but units does not recognize it as a product, and thus the difference. In the future, we may want to offload more stuff to UDUNITS-2 for better speed and compliance. But yeah, that's a separate question for a separate PR.

The only real comment I have is: I think the examples section could benefit from some "real" examples that someone might reasonably encounter; The 1 apple == 2 oranges and person*radian examples confused me more than they clarified how this is meant to be used.

In this PR, I merely translated the previous examples in the now deprecated functions to the new interface. But I agree that we need better examples. I'll think about it.

  • define a dB unit. show how to calculate dB from a ratio of two signal powers (one of which can be noise, which would give SNR).

Note that the bel is already defined by default when units is loaded (here).

Enchufa2 commented 3 years ago

Ok, I've added more meaningful examples: fortnight, currencies and CFUs. If there are no further comments, we can merge this.

Enchufa2 commented 3 years ago

@edzer Can I merge this?

edzer commented 3 years ago

Thanks, looks great!