Issue #233 -- time improvements - #235
Merged
Merged
Conversation
…o format docs: add previously missing changelog entry for time support
Especially on serialization it's important to write the format as the _str variants if the values are actually strings with epoch prefixed-values, simply as that is what existing software (both asdf-astropy, and by extension astropy itself) will accept.
This is effectively equivalent to just the "iso" format. In principle should require a 'T' in the time string but we don't really worry about it and lean towards flexibility; it's more a question for serialization.
Collaborator
Author
|
The proper handling of the Astropy-specific "other_formats" is even weirder than I thought. AFAICT the right approach is to serialize them as "iso" format strings, though the "fits" format is kind of an odd case too because it supports this extended year format (signed, five-digit years) which is actually not "iso" or one of what it considers a "guessable format". Actually, after some experimentation, it turns out this case does not even work properly in Astropy despite what the documentation says: I realize this is an edge case that I shouldn't spend too much time on, but I think this is a good exercise in stress-testing the schema and the specification, because right now I think it's a bit under-cooked when it comes to these Astropy-specific cases, and the behavior for handling them seems only specified in code for now (in asdf_astropy, which converts most of these Astropy-specific formats to ISO format as far as I can tell...except...that isn't always correct). |
After re-reading the schema again and TimeConverter in asdf_astropy I have a better understanding of how base_format is used, and that the "format" property should, if correct within the schema, *never* contain one of the Astropy other-formats. This presents several challenges for how to handle this in the API, but what I settled on is to delete the previously added `asdf_time_t.base_format` field. `asdf_time_t.format` is always the *effective* format (e.g. fits, datetime) as read from the file (taking into account base_format) or specified by the user. The standard "wire" format--what's written in the file to the "format" property is always one of the ASDF-supported formats, and where no other direct representation of that format is possible, we try to convert to an ISO time string, which is close to what asdf_astropy does. There is a hairy corner-case in the case of the "fits" format when it uses extended signed five-digit years. Here we refuse to specify a "format" (only "base_format"). libasdf will handle this correctly; Astropy + asdf_astropy may not but that is a bug there, and one that probably doesn't come up often.
includes gps, galexsec, cxcsec, tai, unix_tai, utime; this is a best effort minimal approach that doesn't account for leap seconds when converting to timestamps, which is documented.
This is really a mess because ASDF has no internal representation for these so always serializes them in "isot" format effectively. This also led to finding a bug in plot_date along similar lines--asdf_astropy can write a plot_date as an ISO format string, but it is also allowed to be a float technically, so we have to be extra careful in round-tripping it in the former case. All the more reason these time formats need to be clarified better because I'm just guessing here as to what the best behavior is really intended to be...
embray
added
enhancement
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Resolves #233 , adding support from time<=1.4.0, and the
base_formatproperty for time>=1.2.In scrutinizing the schemas I noticed that the
isoformat was mishandled--it's just callediso, notiso_time(I think that name might have been accidentally read from the defines).Resolves #208 as well, except for time array handling which is deferred to #237
AI Disclosure
No AI tools used.