search

pvanlaake / CFtime

Work with CF-convention time coordinates and calendars in R
Other
4 stars 0 forks source link

Bug in .parse_timestamp function? #6

Closed lochbika closed 3 months ago

lochbika commented 3 months ago

Hello, I try to run the following code with CFtime 1.3.0.

CFtime::CFtime("hours since 2015-01-16T12:00:00", "gregorian")

which gives me the following error

Error in CFdatum(definition, calendar) : Definition string does not appear to be a CF-compliant time coordinate description: invalid base date specification

As far as I can see, there should be no problem with this definition. Could it be that this is not properly handled in the parsing functions?

Thanks for the help

pvanlaake commented 3 months ago

Hi Kai,

This is a known issue that has been fixed in commit https://github.com/pvanlaake/CFtime/commit/2e0109413ec6ecc7b9d2e4e416da2f265485e0e2.

The problem was that in CFdatum.R the entire definition string was converted to lowercase, including the "T" as the date-time separator (and the "Z" time zone designator). Your definition string thus became "hours since 2015-01-16t12:00:00" and the parser then rejected the "t" separator.

A new release of CFtime will be out in a few weeks and then the problem no longer exists. In the mean time you have two options:

  1. Upgrade to the development version (which has a few other exciting new features, see the NEWS file)
  2. Remove the "T", replacing it with a space: units <- sub("T", " ", units)
lochbika commented 3 months ago

Ok, thanks for the quick reply and the suggestions for workarounds.

Best wishes and keep up the nice work :+1:

pvanlaake commented 3 months ago

Hi Kai,

Version 1.4.0 was just released on CRAN. Enjoy CFtime.

Now preparing release of new NetCDF package ncdfCF. NetCDF in R was never as easy as this.

Patrick

lochbika commented 3 months ago

Thanks, Patrick. I'll go and check out the new netcdf package. Sounds exciting.

Just wanted to share some quick feedback: I know it's a quite young project and things are evolving quickly at this stage, but, I noticed an issue with your release procedure. You introduce backwards compatibility breaking changes in minor versions. For instance, version 1.4 just forgets that the CFsubset function ever existed. Maybe it would be better to create an alias that maps to the new slab function and produces a warning/note before fully deprecating CFsubset. That gives other developers time to adapt. If the new slab function is not compatible with the old one, just release a new major version. Then everyone knows its a major upgrade where special care must be taken before updating.

In any case, let me assure you that I very much appreciate your work here and are thankful for your efforts Best, Kai

pvanlaake commented 3 months ago

Hi Kai,

Thanks for the feedback. I have actually changed the names of a few functions and those are all included in deprecated.R so that calling them generates a warning and it then calls the current function. CFsubset(), however, is a method of the CFtime S4 class and then this approach doesn't work (sorry, don't recall exactly what the problem was).

ncdfCF is out on CRAN. No writing and no groups, that will come over the next releases. So I am not advertising it loudly just yet. But do check it out, you'll be among the first users!

lochbika commented 3 months ago

Excellent, then I definitely missed/didn't get that warning. I tried to build a private package that imports some functions (one is CFsubset) and throws an error during installation that CFsubset is not exported by CFtime when using version 1.4.

So, currently I'm using still on 1.3. But, I'll upgrade soon