joshuaulrich / xts

Extensible time series class that provides uniform handling of many R time series classes by extending zoo.
http://joshuaulrich.github.io/xts/
GNU General Public License v2.0
220 stars 71 forks source link

make Ops.xts more robust when calling add-class #332

Closed msannell closed 4 years ago

msannell commented 4 years ago

I suggest that you change the code in Ops.xts from: .Call('add_class', e, CLASS, PACKAGE="xts") to: e <- .Call('add_class', e, CLASS, PACKAGE="xts")

It won't change the current behavior, but could make it more robust and prevent bugs that could appear if R is changed later.

The problem is that the C function add_class in add-class.c may copy the object e (in the code if(MAYBE_SHARED(x)) x = duplicate(x);) before setting the class attribute. This doesn't currently happen because MAYBE_SHARED(x) is false when add_class is called, but there may be situations in the future where this is not true.

I found this issue when running xts tests in our alternate R interpreter TERR, where the object reference counts are not exactly the same as in R (we're looking into this).

joshuaulrich commented 4 years ago

Thank you for the report and suggestion! I will investigate also.

Can you share the xts tests you were running?

msannell commented 4 years ago

To be clear: I am not seeing any incorrect behavior when running xts in R. The problem only occurred in our separate R-compatible engine (TERR) which doesn't use the exact same refcount/named values as R.

We saw the problem running arithmetic operations on xts objects such as: .xts(101:103,index=1:3) + .xts(201:203, index=2:4)

In R, this returns a correct xts object with class==c("xts","zoo"). In TERR, this returns the same object, except that the class was not set (so it is a matrix).

joshuaulrich commented 4 years ago

Thanks again for your report!