-
Notifications
You must be signed in to change notification settings - Fork 94
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
Bulk update to support CFtime as time management (issue #674) #690
base: main
Are you sure you want to change the base?
Conversation
Here is the section of the test diffs that I get when running
In particular, there are instances where the Are these issues that you will look into? |
This is a PR for integrating
CFtime
intostars
to manage "time" dimensions of NetCDF files (and any others that provide the required metadata). This is a very comprehensive "patch" over >30 files and I am happy to discuss the details if so desired.One of the reasons why the job got a little out of hand is that
CFtime
works fundamentally different from the way that other dimensions are treated instars
. In short,stars
has a reference to aCFtime
instance which holds the actual dimension data. A call likeexpand_dimensions()
will return a reference to that instance rather than the actual dimension values. This necessitated additional conditional evaluation in several higher-level functions, of the kindif (methods::is(x, "CFtime") ...
. I have not fudged any tests or examples and wherever a test or an example referenced a "time" dimension the code works as expected.I made
CFtime
an "Imports" because it is referenced basically throughoutstars
. (And it is the "t" instars
, after all, if the PR is accepted.)In the end it all works fine and no errors are reported on
clean()
(there are plenty of messages on discrepancies of output compared to saved output intests
, but none of that seems to be related to the patch). There is a warning related tocubble
, I do not know how severe this is:Packages
ncmeta
andCFtime
have both had recent upgrades and are on CRAN. This PR was synched with the main branch on 5 June so it is up-to-date as of this writing.