Details
-
Type:
RFC
-
Status: Adopted
-
Resolution: Unresolved
-
Component/s: DM
-
Labels:None
Description
Many of our afw.image FITS I/O methods have many overloads that really only differ (when argument defaults are considered) in the order that arguments are passed. The FITS-reading image constructors have at least 3 overloads, for filename vs. MemFileManager vs afw::fits::Fits. All of the writeFits methods have an extra factor of two for compression options, for a total of 6 writeFits overloads per image type. We also have static readFits methods that duplicate the constructors (in order to provide an consistent interface with the afw::table classes, I suspect), and dedicated FITS-reading helper classes like ImageFitsReader.
Having so many overloads is a problem in its own right, as there's a lot of duplication of both docs and code, but this is a more immediate problem for RFC-817, which would temporarily double the number of most of these overloads (deprecating the existing ones that unnecessarily use shared_ptr while adding new ones that don't).
I'm proposing here that we deprecate and eventually remove all of these from C++, except:
- the FITS-reading helper classes (these are already what we use in Butler)
- a single writeFits method per image class, which would take a afw::fits::Fits instance as its first argument, then image compression arguments (defaulted) and optional header-metadata arguments (defaulted). C++ code that wanted to use a filename or MemFileManager would be expected to (trivially) construct an afw::fits::Fits instance from those.
In Python, we would retain separate writeFits, constructor overloads for filename and afw::fits::Fits (via lambdas in the pybind11 layer), since filename is by far the most common use. All other arguments would be made keyword-only, so their order in the declaration is irrelevant (and incidentally, most calls will get more readable and less error-prone; calls with >3 positional arguments start to worry me). All readFits static methods would be removed.
If this ticket is adopted, I'd want to implement it at the same time as (the rest of) RFC-817, so we'd end up deprecating essentially all of the existing C++ signatures at one time and adding one that replaces all of them (but, I think, breaking very little existing code, especially in Python).
Attachments
Issue Links
- is triggering
-
- To Do
- relates to
-
- Implemented
- mentioned in
-
Page Loading...
-
-
-
-
-
-
-
-
-
Activity
This proposal is intended to be about Image, DecoratedImage, Mask, MaskedImage, and Exposure.
Anyhow, here's a modified proposal to try to incorporate all of the feedback so far:
- Retain writeFits(string) and writeFits(Fits) for all classes in Python and C++ (but make all subsequent arguments kwarg-only in Python and support only one ordering of them in C++, as originally proposed). Drop writeFits(MemFileManager from both languages.
- Retain readFits(string) for all classes in Python and C++. Drop other overloads of readFits in both languages.
- Instead drop all FITS-reading constructors from C++ and Python. This goes beyond the original proposal, because I also like readFits better than constructors as the convenience layer (on top of the more powerful reader helper classes) - this also reduces overall constructor overloading, as well as adding that duck-typing with Catalog. But I'd also like to reserve the option to not do some or all of this even if the RFC is accepted, if doing so looks significantly more disruptive than I expect; I just don't know how much old unit test code we have using these (production code should of course be using the butler), and I don't want to do the substantial work to find out up front.
- Drop MemFileManager constructor overloads from the reader helper classes, and add Fits constructor wrappers to Python, for consistency with other methods. At present, the helper classes cannot be constructed with Fits instances from Python, because when I wrote them I was still fighting the fact that Fits (a scary, not-really-memory-safe thin wrapper around cfitsio) had a pybind11 wrapper at all. The danger of using Fits from Python hasn't changed, but there's not much point in making these classes guard against it while nothing else does, and addressing it should really be a matter for a different, much more ambitious (and not at all planned, at least not by me) RFC.
Krzysztof Findeisen and John Parejko, have you had a chance to look at the updated proposal? When you do, could you chime in on whether you agree/disagree (or anticipate any problems)?
I have no objections.
Krzysztof Findeisen
added a comment - I have no objections. The revised proposal seems reasonable, preserving the most commonly used interfaces while cleaning them up and removing redundancies.
The CCB agreed to recommend it.
I do use Exposure.readFits(filename) (and SourceCatalog.readFits(filename)) in python when mucking about with tests, or trying to understand why a given file has particular contents. So long as there is a way to do that (easily discoverable, as Krzysztof says), I'm happy.
If this is just a proposal about the Image class, then I don't think I've ever used those methods directly, myself.