DM-55726: Fix stamps to add custom metadata on writeFits - #483
Conversation
gmegh
changed the title
gmegh
changed the title
timj
left a comment
timj
left a comment
There was a problem hiding this comment.
I have some comments but I now worry that I gave you bad advice since the code reads like you can already set the metadata for a Stamp and so I'm not sure why we need any new interface at all.
| for key, value in stamp_metadata.items(): | ||
| if key in _EXTENSION_KEYS or key in shared_metadata: | ||
| continue | ||
| hdu_metadata[key] = value |
There was a problem hiding this comment.
This line will not correctly convert multi-valued items from a PropertyList so in theory if a PropertyList is used that has multiple header cards of the same name, you will drop them. The fix would be to check whether stamp_metadata is a dict or PropertyList, convert the dict to PropertyList and then use the .copy() method. If you don't think that is a risk we should at least document this behavior.
| write_archive : `bool`, optional | ||
| Write an archive to store Persistables along with each stamp? | ||
| Default: ``False``. | ||
| stamps_metadata : `~collections.abc.Sequence` \ |
There was a problem hiding this comment.
It would probably be simpler to change this to require PropertyList and not dict, then setStampsMetadata could force dict to PropertyList up front and then the code above could do the safe copy() method call for copying from a known PropertyList.
| Parameters | ||
| ---------- | ||
| stamps_metadata : `~collections.abc.Iterable` \ | ||
| [`~lsst.daf.base.PropertyList`] or `None` |
There was a problem hiding this comment.
This is declared as PropertyList but the lower level APIs take dict. This needs to be clarified (I suggest making this be permissive but forcing the lower level APIs to be PropertyList only).
| self.use_mask, | ||
| self.use_variance, | ||
| self.use_archive, | ||
| stamps_metadata=self.getStampsMetadata(), |
There was a problem hiding this comment.
Should this use the internal attribute so as not to end up with the possibility of it containing the other metadata?
| """ | ||
| if self._stamps_metadata is not None: | ||
| return self._stamps_metadata | ||
| stamps_metadata = [getattr(stamp, "metadata", None) for stamp in self._stamps] |
There was a problem hiding this comment.
I think I'm misunderstanding something. If the individual stamps already have their own metadata why aren't we setting the metadata on each stamp individually and then none of this code is needed? Set the metadata for each stamp where you have each stamp then it should write out properly already shouldn't it? What is the concrete class you are needing this to work on?
This property retrieval is confusing because if you have given override metadata at the Stamps level then you get those overrides, but if you haven't done the override you get the per-Stamp metadata. The setting and the getting is not symmetric.
| if stamp_metadata is None: | ||
| return hdu_metadata | ||
| for key, value in stamp_metadata.items(): | ||
| if key in _EXTENSION_KEYS or key in shared_metadata: |
There was a problem hiding this comment.
I think that if someone has specified a different value for a stamp to that in the primary header, you should be writing that header to the stamp. ie, do not duplicate if it is the same but do duplicate if it differs.
timj
left a comment
timj
left a comment
There was a problem hiding this comment.
Couple of minor comments. Seems like you fixed a really bad off-by-one bug.
| _WRITER_GENERATED_KEYS = frozenset( | ||
| { | ||
| "EXTNAME", "EXTVER", "EXTTYPE", "INHERIT", | ||
| "CRPIX1A", "CRPIX2A", "CRVAL1A", "CRVAL2A", |
There was a problem hiding this comment.
We do not want to be manually keeping a list like this up to date.
lsst.afw.geom.deleteBasicWcsMetadata(hdu_metadata, "A")seems safer (I assume you have made this list because you were getting some keywords printing through on read that were confusing the writer?). I forget how WCS is handled for stamps.
|
|
||
| _WRITER_GENERATED_KEYS = frozenset( | ||
| { | ||
| "EXTNAME", "EXTVER", "EXTTYPE", "INHERIT", |
There was a problem hiding this comment.
We should probably strip these on read since once the MaskedImage is created they are no longer relevant.
| # image extension is used: the mask and variance ones | ||
| # repeat the same EXTVER, and the archive extensions carry | ||
| # no EXTVER at all. | ||
| _metadata = copy.copy(metadata) |
There was a problem hiding this comment.
Do we really want to copy the shared metadata into every HDU? Shouldn't that only be in the primary header?
| maskedImage = masked_image_cls(**stamp_parts[k + 1]) | ||
| archive_element = archive.get(archive_ids[k]) if has_archive else None | ||
| stamps.append(stamp_factory(maskedImage, stamp_metadata[k], k, archive_element)) | ||
| stamps.append(stamp_factory(maskedImage, stamp_metadata[k + 1], k, archive_element)) |
There was a problem hiding this comment.
I hope a test demonstrates this. Pretty bad off-by-one bug it seems.
There was a problem hiding this comment.
Note that it wasn't a bug before because it was a list, but i's now a set labeled by a label EXTVER that is 1-indexed
| # Declared as the union that the constructor accepts, since for a dataclass | ||
| # this annotation is the signature of __init__. After __post_init__ the | ||
| # attribute is always a PropertyList or None. | ||
| metadata: PropertyList | Mapping | None = None |
There was a problem hiding this comment.
Does the code get simpler if we have a default_factory here assigning an empty PropertyList? That might slow down construction and increase memory in the case where we are not storing per-stamp metadata so it might not be a good idea.
2 participants
No description provided.