DM-12740: Make Transform Persistable. #298
Conversation
| std::string getPersistenceName() const override { return getShortClassName(); } | ||
|
|
||
| // implement afw::table::io::Persistable interface | ||
| std::string getPythonModule() const override { return "lsst.afw.geom"; } |
There was a problem hiding this comment.
Should SkyWcs return "lsst.afw.geom" instead of "lsst.afw.geom.skyWcs"?
There was a problem hiding this comment.
Yes. Before now I hadn't thought that distinction would matter, but now that I think about it, exposing the full implementation module here makes the persistence fragile to the kind of refactoring we want to do to shrink the pybind11 build sizes in the future.
There was a problem hiding this comment.
Thanks. I will fix the module name in SkyWcs.cc on tickets/DM-10765
| # Check afw::table::io persistence round-trip | ||
| with lsst.utils.tests.getTempFilePath(".fits") as filename: | ||
| transform.writeFits(filename) | ||
| self.assertTransformsEqual(transform, type(transform).readFits(filename)) |
There was a problem hiding this comment.
Can you add function like the existing transformFromString that figures out the type and returns it?
There was a problem hiding this comment.
Actually, where is that existing transformFromString defined? Is it possible it's not on master yet?
There was a problem hiding this comment.
It is a pure python function defined in lsst/afw/geom/transformFromString.py (I didn't see much point to trying to make a C++ version). It has been on master for a long time. I'm not sure how easily it can be emulated for this case because the reading is done in C++ and one has to read some of the data to figure out the data type to return. That doesn't seem very practical in C++.
There was a problem hiding this comment.
Yeah, I think it'd be tricky to implement. Here are my thoughts on how best to do it:
- If you call
table.io.Persistable.readFitson a FITS file that contains aTransform, it will already return an opaque object that actually holds aTransformof the right type. It'd be easy to call that within a function with a more intuitive name. - If we could expose to pybind11 the fact that
Transforminherits fromtable.io.Persistable, pybind11 would automatically cast that opaque object to the right kind ofTransformon return - but we can't expose that inheritance to pybind11 because hiding it is how we avoid circular dependency problems. - We could get the same behavior by adding a non-templated polymorphic base class between
Persistableand theTransformtemplates (even if it doesn't have any methods); that could be exposed to pybind11, and if we then wrap a trivial C++ function that returns a (say)TransformBasefrom a FITS file, pybind11 will automatically downcast to the right derived/templated type.
I don't think that's worthwhile just for this function, but if TransformBase would be useful for other purposes, it might be worth considering. Because Transform's most important methods depend on its template parameters, TransformBase can't expose a very useful interface, but it could have a few pure virtual methods: hasForward, hasInverse, and writeStream. It would simplify the implementation of the current transformFromString (since that could just wrap a C++ function that returns TransformBase, and pybind11 would do the casting there, too). Let me know what you think.
There was a problem hiding this comment.
Interesting idea. I agree it's probably not worth the bother, but feel free to do it if you are interested. I am not sure how much use we would get out of the capability (we have no users of transformFromString yet, as far as I can tell).
There was a problem hiding this comment.
Ok, I'll skip it for now. Should probably focus on things we know we need rather than things we think we might need.
src/geom/Transform.cc
Outdated
| private: | ||
| TransformPersistenceHelper() : | ||
| schema(), | ||
| bytes(schema.addField<table::Array<std::uint8_t>>("bytes", "an opaque bytestring containing hte transform", "")) |
There was a problem hiding this comment.
"hte" -> "the". Please say more about the format, which is that returned by Transform.writeString, so I was thinking something along the lines of "a bytestring containing the output of Transform.writeString".
SkyWcs says "wcs string representation" which is a bit lean, but at least hints at the format. Feel free to update it to match.
There was a problem hiding this comment.
Is it desirable for the user to think they can interpret the persistence format without our code? That's essentially exposing a new interface, which makes it harder to change that interface in the future.
I think that's unlikely to actually cause a problem in this case, but I do think it's worth thinking about whether opacity is a virtue here.
There was a problem hiding this comment.
Transform.writeString is explicitly is intended for persistence so I think it is safe in this case. I lean towards documenting our use of that format, but I won't insist.
TallJimbo
force-pushed
the
tickets/DM-12740
branch
2 times, most recently
from
8c37ef6
to
d5c0523
Compare
TallJimbo
force-pushed
the
tickets/DM-12740
branch
from
d5c0523
to
8787909
Compare
No description provided.