r-quantities / units

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

Always allow comparisons to logical or numeric NA #336

Closed billdenney closed 1 year ago

billdenney commented 1 year ago

Fix #308

This always allows comparison to numeric or logical NA (and it adds a test for non-NA comparisons, too).

codecov[bot] commented 1 year ago

Codecov Report

Base: 91.54% // Head: 91.72% // Increases project coverage by +0.18% :tada:

Coverage data is based on head (016fd5b) compared to base (3884458). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #336 +/- ## ========================================== + Coverage 91.54% 91.72% +0.18% ========================================== Files 19 19 Lines 934 943 +9 ========================================== + Hits 855 865 +10 + Misses 79 78 -1 ``` | [Impacted Files](https://codecov.io/gh/r-quantities/units/pull/336?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=r-quantities) | Coverage Δ | | |---|---|---| | [R/arith.R](https://codecov.io/gh/r-quantities/units/pull/336/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=r-quantities#diff-Ui9hcml0aC5S) | `96.51% <100.00%> (+0.40%)` | :arrow_up: | | [R/make\_units.R](https://codecov.io/gh/r-quantities/units/pull/336/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=r-quantities#diff-Ui9tYWtlX3VuaXRzLlI=) | `90.67% <0.00%> (+0.84%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=r-quantities). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=r-quantities)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

Enchufa2 commented 1 year ago

About this one, I'm not convinced either that we should treat NA differently. I think too that missing values should have units to allow the comparison, as with any other numeric.

billdenney commented 1 year ago

My main use case is that the way I get data is not always clean. (I'm a heavy contributor to the janitor package for that reason 😄 .)

I will often get data where there is an NA indicator when a measurement is not taken. That NA will not have units associated. And, I may want to compare the NA to the median of the distribution. In my code, I can remove the units to check for NA, but to me, the general idea of "comparison to missing is always missing" holds.

For the use case in the PKNCA library, I currently do many checks for NA. When trying to add units support, I'd need to do some special handling to allow the library to work with and without units. Allowing NA comparison would prevent that need.

That said, if it's philosophically not aligned with the package, I can make the more extensive code changes in my other code.

Enchufa2 commented 1 year ago

Yes, I'm sorry, but philosophically, a data frame column, for example, may be longitude or mass or whatever, and it should have the proper units for comparison independently of whether there is no missing measurement, or some missing measurements, or all missing. Let's keep it in that way for consistency. Thanks for your understanding!