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

Move index attributes to index object #245

Closed
joshuaulrich opened this issue May 29, 2018 · 4 comments
Closed

Move index attributes to index object #245

joshuaulrich opened this issue May 29, 2018 · 4 comments

Comments

@joshuaulrich
Copy link
Owner

The xts class was originally implemented with the index class and index timezone attributes attached to the xts object itself. That means the xts class needs to know about all possible index classes, which makes it difficult to extend xts to use a new index class.

Jeff started to move the index-specific attributes from the xts object to the index object. See the commit message for e4edc53.

o added tzone and tclass functions as aliases to
indexTZ and indexClass, respectively. Eventually
will Deprecate/Defunct the former.

Most places currently set attributes on both objects, because it's not clear which components of xts depend on the index-specific attributes being attached to either object (xts or index).

We would first need to ensure that all attempts to access the index class or timezone are getting and returning values from the index object (note that this applies to R and C code). Then we would need to remove all attempts to set attributes on the xts object... and see what breaks.

@joshuaulrich
Copy link
Owner Author

This is required before #190 can be completed.

joshuaulrich added a commit that referenced this issue May 31, 2018
Mark `indexTZ()` and `indexTZ<-` as deprecated, if only to make their
usage easier to find during testing and reverse dependency checks.
Remove their S3 methods and call their respective "tzone" function
instead.

Replace calls to indexTZ() with calls to tzone(). Remove all uses of
'xts_IndexTZSymbol' in C code, including the macros 'GET_xtsIndexTZ'
and 'SET_xtsIndexTZ'.

Opportunistically remove .indexTZ and tzone attributes from objects
created using prior versions of xts. Also remove "TZ" name attribute
from `tzone<-.xts`.

Rename R/indexTZ.R to R/tzone.R, and man/indexTZ.Rd to man/tzone.Rd.

See #245.
joshuaulrich added a commit that referenced this issue May 31, 2018
Mark `indexClass()` and `indexClass<-` as deprecated, if only to make
their usage easier to find during testing and reverse dependency
checks. Remove their S3 methods and call their respective "tclass"
function instead.

Replace calls to indexClass() with calls to tclass(). Remove all uses
of 'xts_IndexClassSymbol' in C code, including the macros
'GET_xtsIndexClass' and 'SET_xtsIndexClass'.

Opportunistically remove .indexCLASS and tclass attributes from
objects created using prior versions of xts.

Rename R/indexClass.R to R/tclass.R and man/indexClass.Rd to
man/tclass.Rd.

See #245.
joshuaulrich added a commit that referenced this issue Oct 22, 2018
Mark `indexTZ()` and `indexTZ<-` as deprecated, if only to make their
usage easier to find during testing and reverse dependency checks.
Remove their S3 methods and call their respective "tzone" function
instead.

Replace calls to indexTZ() with calls to tzone(). Remove all uses of
'xts_IndexTZSymbol' in C code, including the macros 'GET_xtsIndexTZ'
and 'SET_xtsIndexTZ'.

Opportunistically remove .indexTZ and tzone attributes from objects
created using prior versions of xts. Also remove "TZ" name attribute
from `tzone<-.xts`.

Rename R/indexTZ.R to R/tzone.R, and man/indexTZ.Rd to man/tzone.Rd.

See #245.
joshuaulrich added a commit that referenced this issue Oct 22, 2018
Mark `indexClass()` and `indexClass<-` as deprecated, if only to make
their usage easier to find during testing and reverse dependency
checks. Remove their S3 methods and call their respective "tclass"
function instead.

Replace calls to indexClass() with calls to tclass(). Remove all uses
of 'xts_IndexClassSymbol' in C code, including the macros
'GET_xtsIndexClass' and 'SET_xtsIndexClass'.

Opportunistically remove .indexCLASS and tclass attributes from
objects created using prior versions of xts.

Rename R/indexClass.R to R/tclass.R and man/indexClass.Rd to
man/tclass.Rd.

See #245.
joshuaulrich added a commit that referenced this issue Oct 22, 2018
Mark `indexFormat()` and `indexFormat<-` as deprecated, if only to
make their usage easier to find during testing and reverse dependency
checks. Remove their S3 methods and call their respective "tformat"
functions instead.

Replace calls to indexFormat() with calls to tformat(). Remove all
uses of 'xts_IndexFormatSymbol' in C code, including the macros
'GET_xtsIndexFormat' and 'SET_xtsIndexFormat'.

Opportunistically remove .indexFORMAT attribute from objects created
using prior versions of xts.

Rename R/indexFormat.R to R/tformat.R.

See #245.
joshuaulrich added a commit that referenced this issue Oct 22, 2018
Index attributes will no longer be attached to the xts object itself,
so warn the user if they pass deprecated arguments to the xts
constructor.

See #245.
@joshuaulrich
Copy link
Owner Author

Reverse-dependency checks indicate issues in the following packages: geoSpectral, PRISM.forecast, and RcppXts.

geoSpectral

At least one example fails:

* checking examples ... ERROR
Running examples in 'geoSpectral-Ex.R' failed
The error most likely occurred in:

> ### Name: SpcHeaderAdd
> ### Title: Set a field of the @header slot of a 'SpcHeader' class object
> ### Aliases: SpcHeaderAdd SpcHeaderAdd,SpcHeader-method
>
> ### ** Examples
>
> sp=spc.example_spectra()
Error in validityMethod(as(object, superClass)) :
  tz1.set == tz2.set is not TRUE
Calls: spc.example_spectra ... anyStrings -> isTRUE -> validityMethod -> stopifnot
Execution halted

And at least one test fails for what looks like a similar reason:

> library(testthat)
> library(geoSpectral)
>
> test_check("geoSpectral")
── 1. Error: (unknown) (@test_Spectra_Constructor.R#4)  ────────────────────────
tz1.set == tz2.set is not TRUE
1: spc.example_spectra() at testthat/test_Spectra_Constructor.R:4
2: Spectra(abs, Wavelengths = lbd, Units = Units, ShortName = "a_nap", LongName = "Absorption coefficient by non-algal particles")
3: new("Spectra", out, Spectra = Spectra, Wavelengths = Wavelengths, Units =    Units,
       header = header, ...)
4: initialize(value, ...)
5: initialize(value, ...)
6: validObject(.Object)
7: anyStrings(validityMethod(as(object, superClass)))
8: isTRUE(x)
9: validityMethod(as(object, superClass))
10: stopifnot(tz1.set == tz2.set)

══ testthat results  ═══════════════════════════════════════════════════════════
OK: 0 SKIPPED: 0 FAILED: 1
1. Error: (unknown) (@test_Spectra_Constructor.R#4)

Error: testthat unit tests failed
Execution halted

PRISM.forecast

Example fails:

* checking examples ... ERROR
Running examples in 'PRISM.forecast-Ex.R' failed
The error most likely occurred in:

> ### Name: prism
> ### Title: PRISM function
> ### Aliases: prism
> 
> ### ** Examples
> 
> prism_data = load_5y_search_data('0610')
> data = prism_data$claim.data[1:200] # load claim data from 2006-01-07 to 2009-10-31
> data[200] = NA # delete the data for the latest date and try to nowcast it.
> 
> data.early = prism_data$claim.earlyData # load claim prior to 2006
> GTdata = prism_data$allSearch[1:200] # load Google trend data from 2006-01-07 to 2009-10-31
>
> result = prism(data, data.early, GTdata) # call prism method
Warning in tclass.xts(x) : index does not have a ‘tclass’ attribute
Warning in tclass.xts(x) :
  object does not have a ‘tclass’ or ‘.indexCLASS’ attribute
Error in class(xx) <- cl : attempt to set an attribute on NULL
Calls: prism ... as.matrix -> as.matrix.xts -> index -> index.xts -> .POSIXct
Execution halted

RcppXts

Test fails:

> library(RcppXts)
> X  <- xts(1:4, order.by=Sys.time()+0:3)
> # ...
> xtsRbind(X, X, TRUE)
Error in index.xts(x) : unsupported 'tclass' indexing type:
Calls: <Anonymous> ... coredata -> coredata.xts -> format -> index -> index.xts
In addition: Warning messages:
1: In tclass.xts(x) : index does not have a 'tclass' attribute
2: In tclass.xts(x) :
  object does not have a 'tclass' or '.indexCLASS' attribute
3: In tzone.xts(x) : index does not have a 'tzone' attribute
4: In tzone.xts(x) :
  object does not have a 'tzone' or '.indexTZ' attribute
5: In tzone.xts(x) : index does not have a 'tzone' attribute
6: In tzone.xts(x) :
  object does not have a 'tzone' or '.indexTZ' attribute
7: In tclass.xts(x) : index does not have a 'tclass' attribute
8: In tclass.xts(x) :
  object does not have a 'tclass' or '.indexCLASS' attribute
Execution halted

@joshuaulrich
Copy link
Owner Author

The RcppXts bug is in the xts C function do_rbind_xts:

X  <- xts(1:4, order.by=Sys.time()+0:3)
rbx <- function(..., dup = FALSE, deparse.level = 1) {
  .External("rbindXts", dup = dup, ..., PACKAGE = "xts")
}
rbx(X, X, dup = TRUE)

The PRISM.forecast bug can be replicated with:

data(sample_matrix)
x <- as.xts(sample_matrix)
drop(x[,1])-coredata(x[,1:2])

The geoSpectral bug requires debugging S4, which I'm not particularly good at.

joshuaulrich added a commit that referenced this issue Nov 5, 2018
Calling lengthgets() creates a new object, which will not have the
same attributes as the original object.

Wait to copy the original index attributes to the new index until
after lengthgets() is potentially called.

See #245.
joshuaulrich added a commit that referenced this issue Apr 24, 2019
Mark `indexTZ()` and `indexTZ<-` as deprecated, if only to make their
usage easier to find during testing and reverse dependency checks.
Remove their S3 methods and call their respective "tzone" function
instead.

Replace calls to indexTZ() with calls to tzone(). Remove all uses of
'xts_IndexTZSymbol' in C code, including the macros 'GET_xtsIndexTZ'
and 'SET_xtsIndexTZ'.

Opportunistically remove .indexTZ and tzone attributes from objects
created using prior versions of xts. Also remove "TZ" name attribute
from `tzone<-.xts`.

Rename R/indexTZ.R to R/tzone.R, and man/indexTZ.Rd to man/tzone.Rd.

See #245.
joshuaulrich added a commit that referenced this issue Apr 24, 2019
Mark `indexClass()` and `indexClass<-` as deprecated, if only to make
their usage easier to find during testing and reverse dependency
checks. Remove their S3 methods and call their respective "tclass"
function instead.

Replace calls to indexClass() with calls to tclass(). Remove all uses
of 'xts_IndexClassSymbol' in C code, including the macros
'GET_xtsIndexClass' and 'SET_xtsIndexClass'.

Opportunistically remove .indexCLASS and tclass attributes from
objects created using prior versions of xts.

Rename R/indexClass.R to R/tclass.R and man/indexClass.Rd to
man/tclass.Rd.

See #245.
joshuaulrich added a commit that referenced this issue Apr 24, 2019
Mark `indexFormat()` and `indexFormat<-` as deprecated, if only to
make their usage easier to find during testing and reverse dependency
checks. Remove their S3 methods and call their respective "tformat"
functions instead.

Replace calls to indexFormat() with calls to tformat(). Remove all
uses of 'xts_IndexFormatSymbol' in C code, including the macros
'GET_xtsIndexFormat' and 'SET_xtsIndexFormat'.

Opportunistically remove .indexFORMAT attribute from objects created
using prior versions of xts.

Rename R/indexFormat.R to R/tformat.R.

See #245.
joshuaulrich added a commit that referenced this issue Apr 24, 2019
Index attributes will no longer be attached to the xts object itself,
so warn the user if they pass deprecated arguments to the xts
constructor.

See #245.
joshuaulrich added a commit that referenced this issue Apr 24, 2019
Calling lengthgets() creates a new object, which will not have the
same attributes as the original object.

Wait to copy the original index attributes to the new index until
after lengthgets() is potentially called.

See #245.
@joshuaulrich
Copy link
Owner Author

joshuaulrich commented Apr 28, 2019

It looks like the PRISM.forecast error is due to malformed xts objects being created in the Ops.xts() method. I will create a new issue to document those. That issue will need to be closed before this can be merged.

joshuaulrich added a commit that referenced this issue May 6, 2019
Calling as.matrix() on an xts object without a dim attribute throws an
error because ncol() is called on the xts object. ncol() only works if
the object has a dim attribute, while NCOL() also works on vectors.

Fixes #294. See #245.
joshuaulrich added a commit that referenced this issue May 6, 2019
We need an all.equal.xts() method to handle the case where all.equal()
is called with one xts object that has index attributes on the object
and one that only has them on the index. This can cause unit test
failures, and did in the "argo" package.

The function only removes the index attributes from either (or both)
objects before calling NextMethod(). I avoided the urge to check the
attributes attached to the object and index for consistency, and chose
what seemed to be a simpler approach.

See #245.
joshuaulrich added a commit that referenced this issue May 12, 2019
Silly bug. Add tests for tclass(), tformat(), and tzone().

See #245.
joshuaulrich added a commit that referenced this issue May 12, 2019
Check for .indexFORMAT and tformat passed to the constructor via
'...'. Warn that .indexFORMAT is deprecated, but still use it. We
will raise this to an error in a later version.

This means the special handling for Ops.xts() is now handled as part
of the more general case. It's also not strictly needed, because
Ops.xts() no longer passes .indexFORMAT to the constructor.

Remove these two elements from '...' before attaching the remaining
elements to the result (as user attributes).

See #245.
joshuaulrich added a commit that referenced this issue May 12, 2019
Check for .indexCLASS and tclass passed to the constructor via '...'.
Warn that .indexCLASS is deprecated, but still use it. We will raise
this to an error in a later version.

Also warn if the tclass attribute on the index is not the same as the
tclass argument default value ("POSIXct", "POSIXt"). Current behavior
over-rides the index tclass attribute with the default argument value.
This seems like a bug, but it may break existing code, so warn before
changing or converting to an error.

Remove these two elements from '...' before attaching the remaining
elements to the result (as user attributes).

Remove the indexClass() call from the checkXtsClass() test function,
since indexClass() is deprecated.

See #245.
joshuaulrich added a commit that referenced this issue May 12, 2019
Check for .indexTZ and tzone passed to the constructor via '...'. Warn
that .indexTZ is deprecated and will be ignored. This is consistent
with current behavior, but differs from the other two attributes that
can be over-ridden via arguments passed via '...'.

Remove these two elements from '...' before attaching the remaining
elements to the result (as user attributes).

Remove the indexTZ() call from the checkXtsTz() test function, since
indexTZ() is deprecated.

See #245.
joshuaulrich added a commit that referenced this issue May 12, 2019
We need to drop any index attributes (i.e deprecated .index* and
tformat) passed via '...', but retain any user attributes and standard
attributes like dim, dimnames, etc.

Note that tclass and tzone are not included in the dropped attributes
because they are formal arguments to the constructor, and therefore do
not appear in '...'.

See #245.
@joshuaulrich joshuaulrich added this to the 0.12-0 milestone Jul 4, 2019
joshuaulrich added a commit that referenced this issue Jan 20, 2020
The current behavior ignores the tclass attribute on the index passed
to .xts(). This is a bug, but throwing a warning breaks existing
packages and causes errors in their CRAN checks. Reinstate the warning
after fixing dependencies.

See #245.
joshuaulrich added a commit that referenced this issue Feb 11, 2020
Missed this change while working on #245. Thanks to Claymore Marshall
for catching it!
joshuaulrich added a commit that referenced this issue Apr 4, 2020
When e1 and e2 do not have identical indexes, we overwrite them with
the result of merge(..., retclass = FALSE). That means they no longer
have a 'class' attribute, but they still have their 'index' attribute.
Next, we calculate 'e' using the Ops method for 'matrix' objects,
which means 'e' won't have an index attribute.

Also add default methods for `tclass` and `tclass<-`. These were
missed during the updates for #245.

Fixes #322.
@joshuaulrich joshuaulrich moved this from To Do to Finished in Support nanosecond resolution May 23, 2021
joshuaulrich added a commit that referenced this issue Oct 23, 2022
indexClass() was deprecated when all index attributes were moved from
the xts object directly to the index.

See #245.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

No branches or pull requests

1 participant