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

Version aware construction #188

Closed
moodymudskipper opened this issue Jun 6, 2023 · 3 comments
Closed

Version aware construction #188

moodymudskipper opened this issue Jun 6, 2023 · 3 comments

Comments

@moodymudskipper
Copy link
Collaborator

cc @krlmlr

I knew it would come but I didn't expect it to come so fast!

By essence {constructive} is implementation dependent, this means that if an object is implemented differently with a different version of a package, or of base R, we need to construct it differently.

This means our tests also need to be version aware.

Here the workflows already show problems due to changes in base R 4.3 https://cran.r-project.org/doc/manuals/r-release/NEWS.html

  • Objects of class "POSIXlt" created in this version of R always have 11 components: component zone is always set, and component gmtoff is set for times in UTC and usually set on the (almost all) platforms which have C-level support, otherwise is NA.
  • More experimentally, a "POSIXlt" object may have an attribute "balanced" indicating if it is known to be filled or fully balanced.
  • When ts() creates a multivariate time series, "mts", it also inherits from "array" now, and is.mts() is documented and stricter.

We have things like :

# interesting to see how constructive constructs POSIXlt on top of POSIXct in R 4.3
construct(as.POSIXlt(.leap.seconds[1:4]))
#>  Output
#> -   as.POSIXlt(c("1972-07-01", "1973-01-01", "1974-01-01", "1975-01-01"), tz = "GMT")
#> +   as.POSIXlt(as.POSIXct(c("1972-07-01", "1973-01-01", "1974-01-01", "1975-01-01"), tz = "GMT")) 

# new "balanced"attribute
construct(as.POSIXlt("2022-01-01 01:00:00", tz = "UTC"))
#>    Output
#>  -   as.POSIXlt("2022-01-01 01:00:00", tz = "UTC")
#>  +   as.POSIXlt(as.POSIXct("2022-01-01 01:00:00", tz = "UTC")) |>
#>  +     structure(balanced = TRUE)

#  the mts tests are also affected 
      structure(
        dim = c(3L, 3L),
        dimnames = list(NULL, c("Series 1", "Series 2", "Series 3")),
-       tsp = c(1961, 0x1.ea4aaaaaaaaabp+10, 12),
+       tsp = c(1961, 1961.166666666666742458, 12),
-       class = c("mts", "ts", "matrix")
+       class = c("mts", "ts", "matrix", "array")
      )

The good news is that constructive always succeeds to construct the object, in the case of mts the idiomatic form is even still correct, there is nothing to change.

I think R versions and package versions might be stored into options, and we default on whatever is installed. So we can simulate a version, and don't clutter the api.

This issue is about settling on a general approach, other issues will address the quick fixes

@krlmlr
Copy link
Contributor

krlmlr commented Jun 7, 2023

As a first approximation, can we target the newest package version and issue a warning/error if it doesn't match what's installed?

@moodymudskipper
Copy link
Collaborator Author

That would be a lot of warnings, most people don't have everything up to date. And it's a legit use case to construct objects from an old RDS file with a new R, or the opposite, so I don't think failing is appropriate, the construction is succesful, just not necessarily idiomatic .

But maybe as part of #169, and as part of construct_issues()

is_corrupted_POSIXlt() might not only fail on a corrupted object as it does now (or should do), but also assess if the format of the object is compatible with what's installed. We'd store this info in the object for it be picked up by construct_info() or construct_issues().

@moodymudskipper
Copy link
Collaborator Author

We have an example for citFooter and citHeader now

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

No branches or pull requests

2 participants