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

Issue printing date string #1417

Closed
WardF opened this issue Jun 17, 2019 · 14 comments · Fixed by #1868
Closed

Issue printing date string #1417

WardF opened this issue Jun 17, 2019 · 14 comments · Fixed by #1868
Assignees
Milestone

Comments

@WardF
Copy link
Member

WardF commented Jun 17, 2019

The following issue was reported to us. Using the attached file, we see that the printout is assuming the starting offset is from 00:00:00UTC, when it should be from 03:00:00UTC

Issue as reported

ncdump -t on the attached file prints timestamps starting with
"2018-03-01 21", whereas it should be "2018-03-02".

ncdump is from netcdf library coming with Ubuntu 16.04.
(version 4.4.0 of Mar 29 2016 11:41:40)

Could you please check if the issue is reproducible in the current version?
Or is there something non-standard in the way the time dimension
specified in the file?

cdo and panoply are perfectly capable to interpret such time axis
right.

@WardF WardF added this to the 4.7.1 milestone Jun 17, 2019
@WardF WardF self-assigned this Jun 17, 2019
@DennisHeimbigner
Copy link
Collaborator

Is this related to e-support WSC-538180?

@Dave-Allured
Copy link
Contributor

Is this related to e-support WSC-538180?

It is not related. WSC-538180 is about printing time string when number of seconds is a tiny amount smaller than 60. The current issue is a coarse error of 3 hours.

@Dave-Allured
Copy link
Contributor

I can reproduce this behavior with ncdump in the current version, 4.7.0.

I made a second test file using the reporter's data values, and a current 4.7.0 version of ncgen. The incorrect behavior remains exactly the same, in the context of several differences:

  • ncdump and library versions 4.4.0 vs. 4.7.0.
  • Files generated with netcdf 4.4.1.1 vs. 4.7.0.
  • Files generated with hdf5 1.10.0 vs. 1.10.5.
  • Superblock versions 2 vs. 0.
  • Time data types are integer vs. double.

@Dave-Allured
Copy link
Contributor

Okay, this is related to the units string and its time zone indicator. If you make this change:
time:units = "seconds since 2018-03-01 03:00:00 UTC"
to
time:units = "seconds since 2018-03-01 03:00:00"
then the printed time stamps are what the user expected, starting with simply "2018-03-02".

I am not familiar with the proper behavior of the time zone modifiers, or the default time zone assumption. Can anyone else comment on this?

@Dave-Allured
Copy link
Contributor

The original reporter asked "Or is there something non-standard in the way the time dimension
specified in the file?". Maybe you could ask them whether omitting UTC is an acceptable solution.

@WardF WardF modified the milestones: 4.7.1, 4.7.2 Oct 15, 2019
@WardF WardF modified the milestones: 4.7.2, 4.7.3 Oct 28, 2019
@rkouznetsov
Copy link
Contributor

Thank you for the responses and sorry for long silence. Removing UTC would be a workaround, although, I would like to have that UTC mark clearly indicating the absolute time of the time satmps.
To me UTC is very natural scale for observations/models, however I have some experience sharing data with other communities, and these self-obvious things might be very different for people with different background...

Ideal resolution to my view would be to arrange timezone-aware printing at least for Zulu time in case the timezone explicitly specified:
2018-03-02T00Z, 2018-03-02T01Z etc
I wonder how difficult to implement can it be. In any case ncdump should never do the timezone conversion to local time.

@Dave-Allured
Copy link
Contributor

It turns out this is a parser bug, not a simple timezone problem. With "UTC" at the end of the unit string, your initial time of day 03:00:00 is set to zero by mistake. This explains the 3-hour offset. You would never notice this problem for most files that have units time of day = 00:00:00.

Here is a bug fix for libdispatch/nctime.c, against the 4.7.3 release version. Please consider this for the next release.

nctime-parser-fix.from-4.7.3.zip

@Dave-Allured
Copy link
Contributor

The answer for "I am not familiar with the proper behavior of the time zone modifiers" is that the time zone suffix is always ignored by ncdump -t. Current behavior is to interpret time coordinates based only on the date and time of day in the units string. No time zone offset is ever applied. The current bug is an unintended exception.

Another way of saying this is that if your base units include a time zone, then the dates and times from ncdump -t will also be in the same time zone, except no suffix is ever printed.

@Dave-Allured
Copy link
Contributor

Ideal resolution to my view would be to arrange timezone-aware printing at least for Zulu time in case the timezone explicitly specified:
2018-03-02T00Z, 2018-03-02T01Z etc
I wonder how difficult to implement can it be.

It should be pretty easy to copy the identical suffix from the base unit string to printed dates and times. Today I was focused on the bug fix, and I did not think about changing the printing.

@DennisHeimbigner
Copy link
Collaborator

Ward - Did Dave's fix ever get incorporated?

@Dave-Allured
Copy link
Contributor

@DennisHeimbigner @WardF, this fix is not in current master (2020 October 14), and I do not see a PR for it. I am ready to push a new PR from my remote machine, but I am getting error 403 from git push. Please enable push access for me, or advise a different strategy. I am a git newbie, your advice is appreciated. Thanks.

As said before, my current change fixes the parser bug (and the misinterpreted time zone), but does not address the other request to add printed time zones to ncdump.

@WardF
Copy link
Member Author

WardF commented Oct 14, 2020

Hi Dave, the standard process is to fork the repository, incorporate your changes into the forked repo, and then issue a pull request (PR). Adding write/push access outside of the organization isn't something I'm able to enable.

@Dave-Allured
Copy link
Contributor

For future reference, here is a summary of the date string parser bug. Affected versions are netcdf-c 4.2.0 through 4.7.4 (currently).

This affects time unit strings that include both the optional time-of-day field, and the optional time zone field. In 4.2.0, support was added for the ISO 8601 upper-case "T" separator between the date and time-of-day, in place of the UDUNITS traditional space separator. Consider two examples, traditional and ISO 8601:

time:units = "seconds since 2018-03-01 03:00:00 UTC"
time:units = "seconds since 2018-03-01T03:00:00 UTC"

The original parser code did not anticipate the first case including the optional time zone. It gave priority to the first upper-case "T", rather than a space following the date. Therefore, any time zone suffix containing upper-case "T" triggered the bug. UTC, EST, EDT, whatever. Note this only happens when there is a space separator between date and time, followed by a time zone containing upper-case "T".

The date string was then parsed incorrectly as follows.

  • Date substring was parsed correctly, up to the first space separator.
  • The real time-of-day was treated as stray characters following date substring, and ignored.
  • Substring following upper-case "T" was parsed as time-of-day, in this case "C".
  • Parsing of invalid time-of-day ended at the first non-digit, in this case "C".
  • Result: time-of-day = 0. QED.

Also notice that when time-of-day substring is 00:00:00, the most common case, incorrect parsing is hidden because the invalid result of zero is the same as the correct result. The problem appears only with non-zero time-of-day substring.

The bug fix treats both space and upper-case "T" with equal priority, immediately following the date substring.

@czender
Copy link
Contributor

czender commented Jan 13, 2021

Thanks, @Dave-Allured. Good to know that has been fixed.

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

Successfully merging a pull request may close this issue.

5 participants