Repository navigation
Serialize all astropy core objects to FITS - #6912
Conversation
|
Hi there @taldcroft 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃. Everything looks good from my point of view! 👍 If there are any issues with this message, please report them here. |
|
Hmm, remarkably similar to how ESO-MIDAS does it: They do seem to get around the white-space issue in adding backslashes to the end of the cards: I don't know if |
|
Unlike ECSV, this is not intended to be human readable. Here it is without encoding: However, very good point that it is actually preserving leading spaces and so I can cut the one-line YAML by hand and put in EOL markers to preserve trailing space. Will do, I wasn't exactly happy about the encoding. In case it isn't obvious, we definitely want to use the compact YAML format here because we cannot assume that the nice "human readable" form will always fit within the width of a COMMENT card. |
|
OK, now it looks like this, which is readable by sufficiently persistent humans: |
|
I guess if we really wanted the fully human readable format it would be possible to do the line extensions only for lines beyond 70 characters. I sort of think of FITS as a binary format that is not meant for direct human inspection, and making the parsing more difficult will make it harder for other (non-astropy) implementations to support this. |
|
I worry a little about the case where someone reads in the file into another program, removes a column or renames one and writes it back then reads it into astropy. Maybe we need a checksum of the data so that we can discard the YAML if the checksum disagrees? |
|
@taldcroft - that looks good; agreed it should be the compact form, but now someone just looking at the file at least has a chance to understand what is there! Bit more generally, as you know I have been advocating |
|
Great idea! Having a human readable format is important, so not base64 blob please 😱 |
19d6171 to
ddaa799
Compare
|
OK it's fully-human-readable now, and passing tests. |
taldcroft
changed the title
|
@taldcroft - now had a closer look and really liked it. But I noticed that A possible extension of this - for another PR - is to use the new information to ensure no |
|
@mhvk - early in the work I had gotten the idea that TimeDelta was supported by the Time FITS machinery, although it seemed somewhat broken. But looking closer I don't see any tests, and the fact that I just commented out the TimeDelta stuff reflected that I wasn't quite sure of this. Good catch, so now 8e70992 puts back support and this is good! |
|
For the record, here is the current header for a somewhat complicated example with Time, Quantity, EarthLocation and SkyCoord: |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
This really is great. I remain amazed at how nicely the work we did for ecsv is translated here. Only minor comments.
| """Encode a Table ``tbl`` that may have mixin columns to a Table with only | ||
| astropy Columns + appropriate meta-data to allow subsequent decoding. | ||
| """ | ||
| # If PyYAML is not available then check to see if there are any mixin cols |
There was a problem hiding this comment.
I thought for quite a bit about whether we should not warn, and fe;t that perhaps we should warn if loss of information can be prevented by installing yaml.
But definitely only when actually necessary: so, I would move this test below.
There was a problem hiding this comment.
Done. But I prefer to keep the logic here. This is mostly for code clarity, i.e. being explicit right away about the PyYAML dependence. Also if there is no YAML then there is no reason to bother with going through the serialize._represent_mixins_as_columns(...) step.
| try: | ||
| encode_tbl.meta = {ser_col: encode_tbl.meta[ser_col]} | ||
| meta_yaml_lines = meta.get_yaml_from_table(encode_tbl) | ||
| finally: |
There was a problem hiding this comment.
You could add the yaml import test before the try, but might also consider adding an except ImportError here (true EAFP instead of LBYL).
There was a problem hiding this comment.
As from above I think the way it is written now is a bit easier to understand and more efficient.
|
|
||
| # Time column reference position | ||
| if col.location is None: | ||
| if getattr(col, 'location', None) is None: |
There was a problem hiding this comment.
Is this a left-over of trying to get TimeDelta to work? If so, probably best to remove it again. Time should always have .location.
| a2 = {} | ||
|
|
||
| if isinstance(a1, np.ndarray) and a1.dtype.kind == 'f': | ||
| assert quantity_allclose(a1, a2, rtol=1e-10) |
There was a problem hiding this comment.
More out of curiosity, why don't floats round-trip?
There was a problem hiding this comment.
Good catch, that is from ECSV where you might be comparing ASCII-serialized values. I changed this here and it seems to be passing tests.
| for name, col in t.columns.items(): | ||
| col2 = t2[name] | ||
|
|
||
| # Special-case Time, which does not yet support round-tripping |
There was a problem hiding this comment.
In principle, I guess this could be added to the additional information stored. Maybe raise a new issue so we do not forget?
There was a problem hiding this comment.
In principle, but we should just do this right and use a column header keyword in the TimeFITS machinery.
There was a problem hiding this comment.
Note that there is a little issue because for Time the format attribute is a native attribute that is different from Time.info.format (with the latter not really having any meaning in this case).
So right now the Time object is ignored in the serialization process to allow for TimeFITS to do its job, and only the info gets stored in meta. Finessing this is not so easy. So again, just do it right in TimeFITS.
| compare_class = True | ||
|
|
||
| if isinstance(col.info, QuantityInfo): | ||
| # Downgrade Quantity to Column + unit |
There was a problem hiding this comment.
Probably related to previous comment: shouldn't we be able to keep this information? It is, in principle, present in the yaml, no? (again can be for another issue).
There was a problem hiding this comment.
The comment is maybe misleading (indeed I forgot exactly where this was headed). All the info attributes are still there and tested, but what is missing are specific class-related attributes like value or wrap_angle (in the case of Longitude). So basically I think this is OK since Column cannot have a wrap_angle.
However I noticed that the data values were not being compared in this case (also for ECSV) so I fixed that.
|
@mhvk - I think I addressed all your comments. |
|
I think this looks great, thanks for addressing the nitpicks; this does need a rebase now... Just to clarify, currently, we do save all the information from |
Yup!
I actually did try this for fun (make info However, I think we could put in a special case to basically set |
80e859b to
40a6ec7
Compare
|
Ah, great. The hack could also work by having |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Ready to go in once CI passes!
mhvk
added
zzz 💤 merge-when-ci-passes
|
@taldcroft - This one is still missing from the what's new: http://docs.astropy.org/en/latest/whatsnew/3.0.html#what-s-new-in-astropy-3-0 Could you please add some text about it? |
|
Copy, will do. |
|
@taldcroft - Apologies for coming late to the discussion. Thanks for adding this! I tried it for the first time now, and wanted to ask if those are features or bugs: Calling Finally, if you call See examples here: https://github.com/gammapy/gammapy/pull/1358/files#diff-21886c0af8e866d3b74e6042ab6761bfR123 Let me know if opening a new issue with those points is useful. |
This is a proof of concept for serializing all astropy core objects to FITS. It heavily leverages the machinery used for ECSV, and the concept is pretty much identical. HDF5 can be done in much the same way.
For writing the process is to encode a table with mixins into a table with only standard Columns + appropriate YAML-encoded metadata. That metadata has all the information needed to fully-reconstruct the mixin columns.
The key question is how to store the YAML-encoded metadata in FITS. This POC takes a simple approach and just puts it into FITS COMMENT cards, contained within two sentinel strings to allow being able to reliable identify such metadata in a FITS file. This is not the only approach, but it is pretty simple and non-invasive. See code comments for an implementation detail that required base64 encoding of the YAML.
The requirement here is that this process is minimally impacting to output FITS files. The final implementation would be careful to cause no change to the output FITS file for a Table that has no mixin columns. That is not currently the case in this POC.
Reading is just the reverse, and the Columns-only table + metadata are decoded into a Table with mixin columns.
This works right now: