[ and [[ keep attributes

[krlmlr]

This would be easier and faster if vctrs was an imported package, because we could then use vec_restore().

Do you see strong reasons against importing vctrs? We don't necessarily need to change the class hierarchy, although by inheriting from "vctrs_vctr" a lot of code here would be obsolete or could be changed.

[codecov[bot]]

Codecov Report

Merging #275 (597861d) into master (dcbe2dd) will increase coverage by 0.00%. The diff coverage is 100.00%.

:exclamation: Current head 597861d differs from pull request most recent head fc61144. Consider uploading reports for the commit fc61144 to get more accurate results

@@           Coverage Diff           @@
##           master     #275   +/-   ##
=======================================
  Coverage   94.40%   94.41%           
=======================================
  Files          18       18           
  Lines         876      877    +1     
=======================================
+ Hits          827      828    +1     
  Misses         49       49           

Impacted Files	Coverage Δ
R/conversion.R	87.95% <100.00%> (-0.29%)	:arrow_down:
R/make_units.R	90.08% <100.00%> (+0.25%)	: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 9039083...fc61144. Read the comment docs.

[Enchufa2]

Thanks. What is the interest in preserving other attributes? This PR breaks quantities.

About importing vctrs, we would like to remain compatible, but also to work independently with minimum dependencies.

[edzer]

I agree that minimizing the number of hard dependencies is critical for this package. Also, vctrs seems to be pretty dynamic so this would keep everyone quite busy. Once vctrs has reached stability and is rock-solid, and importing it would bring strong advantages (simplifying code could be one!) we could reconsider this.

[krlmlr]

Fair enough.

The motivation here is to support custom formatting for numbers with units, as a follow-up to #273. See https://pillar.r-lib.org/dev/articles/numbers.html#units for a specific example.

I wasn't aware that the "units" class is actually intended to be used as mixin to other classes such as "errors". In this case it makes much more sense to only carry over the attributes that we care about. Once we've settled on attribute names, I'll update the PR.

[Enchufa2]

I see. And wouldn't make more sense to prepend a new class---say, pillar---to take care of those attributes independently of what the main class does? That would be much more robust to changes here and in other packages. Within the r-quantities framework, errors and units are just responsible for their own attributes, and the quantities package coordinates them without expecting any one of them to be aware of the other. For us this is much easier to maintain.

[krlmlr]

We could also treat units and quantities as main classes if they support wrapping of classed objects. This means replacing (at least in units) most of not all instances of unclass() with a call to strip_class() :

strip_class <- function(x) {
  if (identical(x, "units")) {
    unclass(x)
  } else {
    class(x) <- setdiff(class(x), "units")
    x
  }
}

Would you support a change like this?

The alternative -- prepending a class -- doesn't look useful at the first glance.

In a branch in pillar I have:

library(pillar)
library(units)
#> udunits database from /usr/share/xml/udunits/udunits2.xml

set_units(1:3, km) + set_units(1:3, m)
#> Units: [km]
#> [1] 1.001 2.002 3.003
num(1:3, notation = "sci") + 1:3
#> <tibble_num(sci)[3]>
#> [1] 2.00e0 4.00e0 6.00e0

# Doesn't work, looks difficult to fix
num(set_units(1:3, km), notation = "sci") + set_units(1:3, m)
#> Warning: Incompatible methods ("+.vctrs_vctr", "Ops.units") for "+"
#> <tibble_num(sci)[3]>
#> [1] 2.00e0 4.00e0 6.00e0

# Maybe...
set_units.tibble_num <- function(x, ...) {
  unclassed <- x
  class(unclassed) <- NULL
  out <- set_units(unclassed, ...)
  class(out) <- c(class(out), class(x))
  out
}

set_units(num(1:3, notation = "sci"), km)
#> Units: [km]
#> <tibble_num(sci)[3]>
#> [1] 1.00e0 2.00e0 3.00e0

# Ops.units uses unclass() but shouldn't?
set_units(num(1:3), km) + set_units(1:3, m)
#> Units: [km]
#> [1] 1.001 2.002 3.003

^{Created on 2021-03-24 by the reprex package (v1.0.0)}

It would be much easier if units preserved the "pillar" attribute on subsetting, which contains only information related to display. Not perfect object-oriented programming, just much easier and achieves the desired result. I'll update the PR.

[Enchufa2]

Where is this num constructor/class? I would like to play with it. I would like to better understand the set of use cases you have in mind for them, and what's the role of pillar and tibble in them. Because, from the initial proposal, I understood that you were just interested in defining formatting options in tibbles, which I see as the last step of an analysis, but then I see that you are also operating with num (!). So you could end up with something e.g. like

num(1:3, label = "€") + num(1:3, label = "$")

which makes no sense to me.

[krlmlr]

Thanks. It's in the docs-num branch in pillar.

I'd like to argue that applying formatting early in the analysis solves a few problems:

• Better display along the way if the standard formatting doesn't work for one reason or another
• Display format is automatically applied to all end results: plots, tables, ...

When combining two num()s, the LHS wins. The currency example is misleading, I agree -- we could throw an error if both LHS and RHS have different labels.

[Enchufa2]

Ok, being aware of the pillar attribute is a sensible thing to do, because we provide a pillar_shaft method, and this change is compatible upstream with quantities. If that is enough for the purposes of formatting, I'm ok with this change. However,

• one concern is that setting a label should be incompatible with units, which could only be solved in set_num_opts.
• another concern is how these formatting options would play with the errors class, but that's another battle.

If @edzer has no further concerns/comments, I can merge this.

[krlmlr]

• I agree that the label provided by units should have precedence.
• For errors I would suggest a compound pillar where the number is formatted as usual and the error is a separate pillar shaft class. I can provide guidance or a pull request.

[Enchufa2]

Merging, thanks!

For errors, some guidance on how a compound pillar looks like would be helpful.

[krlmlr]

See pillar:::new_array_pillar() for an example: arrays show their first slice and a continuation marker. This looks a little too cumbersome for now, we might provide helpers in pillar.

[krlmlr]

Thanks for taking this! The "pillar" attribute looks a little ugly right now when a units object is printed, I'll send another PR soon.

[Enchufa2]

Unless we want something fancier, just dropping the attribute before calling NextMethod works (https://github.com/r-quantities/units/commit/1d6260cc432373695fd6b9fcc546dd5acc09f3c5).