DM-25786: Add YAML support for Regions and Pixelization #24
Conversation
python/lsst/sphgeom/_yaml.py
Outdated
| # Register all the regions classes with the same constructor and representer | ||
| for region_class in (Region, ConvexPolygon, Ellipse, Circle, Box): | ||
| yaml.add_representer(region_class, region_representer) | ||
|
|
||
| for loader in YamlLoaders: | ||
| yaml.add_constructor("lsst.sphgeom.Region", region_constructor, Loader=loader) |
There was a problem hiding this comment.
Consider adding a comment explaining that you don't need the other classes because Region.decode handles getting the right subclass. This was not obvious to me.
| b = yaml.safe_load(yaml.dump(a)) | ||
| self.assertEqual(a, b) |
There was a problem hiding this comment.
This repetition makes me wonder if it's worth having a general "test yaml" utility somewhere. That would also make it easier to test a wider range of states, for classes that need them (I realize Region does not, because the underlying persistence code is already tested elsewhere).
There was a problem hiding this comment.
There is a lot of repetition in the tests. I was trying to keep this ticket small to get you something quickly and not let it blow up into a full on test refactoring.
python/lsst/sphgeom/_yaml.py
Outdated
| for region_class in (Region, ConvexPolygon, Ellipse, Circle, Box): | ||
| yaml.add_representer(region_class, region_representer) |
There was a problem hiding this comment.
Region is an abstract class (at least in C++? 😨). Why does it need to have a representer registered?
There was a problem hiding this comment.
Region is an abstract class in Python, too, in the sense that it cannot be directly instantiated, but I'd be quite surprised if it used the ABCMeta metaclass that pure-Python ABCs use. I don't know anything about yaml representers, but it is possible for Python code to get what appears to be a base a Region instance there is a concrete concrete C++ subclass was not exposed to pybind11. I don't know if there are any such subclasses in the wild in this hierarchy.
There was a problem hiding this comment.
I'll drop it. It shouldn't make a difference.
python/lsst/sphgeom/_yaml.py
Outdated
| def pixel_representer(dumper, data, pixelScheme): | ||
| """Represent a pixelization in YAML | ||
|
|
||
| Stored as the pixelization level. Use a mapping for more clarity. |
There was a problem hiding this comment.
Please reword to "Uses a mapping". "Use" sounds like an instruction to either the user or a future developer.
More clarity than what?
There was a problem hiding this comment.
Since these are custom functions, and not PyYAML hooks, please provide a full description of the parameters.
There was a problem hiding this comment.
The "more clarity" was a note to myself that scalar would be sufficient but a mapping with a level keyword is more explicit. I'll update. I could make the same change for the region serialization as well and use a mapping with a key "encoded" or something and value the hex string rather than a scalar with the hex string directly.
python/lsst/sphgeom/_yaml.py
Outdated
|
|
||
| yaml.add_representer(pixelSchemeCls, representer) | ||
| for loader in YamlLoaders: | ||
| yaml.add_constructor(f"lsst.sphgeom.{pixelSchemeCls.__name__}", constructor, Loader=loader) |
There was a problem hiding this comment.
Will this be a problem if there's a pixelization class that's not at the sphgeom top level? It might be better to get the fully qualified name, then use that here and for pixel_representer.
There was a problem hiding this comment.
It works for what we have now and it doesn't look like healpix should be implemented differently. If it does we can change it later.
Uses the encode/decode method and stores the encoded bytes as a hex string scalar in the YAML.
No description provided.