Refactor write/read methods to delegate to new Writer/Reader interface. #170
Conversation
TallJimbo
force-pushed
the
writers
branch
13 times, most recently
from
e6e0505
to
7522d8b
Compare
This should exactly preserve the previous file format via FitsWriter; the only difference I'm aware of is that the Piff version is now written to the headers of all HDUs, rather than just some.
There was a problem hiding this comment.
One open question is that I'm not sure if I should do something about the Output class (I have not yet), which looks like it was intended to do some of the same file-type dispatch as the new Writer.open method, but going by code comments never really grew into that role.
|
This is now ready for at least preliminary review, @rmjarvis. Apparent test failure is actually just hitting the codecov rate limit (sorry!), and it should resolve itself if kicked in a few hours. Hopefully nothing unexpected turns up in the rest of the build matrix. |
piff/readers.py
Outdated
| yield reader | ||
| return | ||
| else: | ||
| raise NotImplementedError(f"No reader for extension {ext!r}.") |
There was a problem hiding this comment.
I'm not a big fan of pure Abstract Base Classes in Python. I find them rather clunky, and they don't really serve the kind of purpose that they do in C++ for instance.
And in this case, the base class seems completely unnecessary. What purpose does this function serve? AFAIU, you aren't planning to extend this function to use some other readers with different file types. You will have a different Reader class that you will initialize differently. I.e. not opening a file.
So the current implementations should just use FitsReader.open() to open the fits file. And you'll have a different context that uses whatever object you are serializing to, not a file_name.
Also, removing the base class saves you from having to write tests for all these NotImplementedErrors that are currently uncovered.
There was a problem hiding this comment.
I'm happy to drop the open method.
I like ABCs as a place to hang docstrings (most of the time derived-class methods can just inherit them) and more generally document the interface that users and implementers are expected to satisfy. But I'm happy to remove it if it's not the pattern you prefer, and just move the docstrings over to the FITS implementations.
piff/writers.py
Outdated
| header = self._header | ||
| self._fits.write_image( | ||
| array, extname=self.get_full_name(name), header=self._header | ||
| ) |
There was a problem hiding this comment.
This method seems not to be used? We don't have any PSF types that write images, so I think for now write_table is all we need. Although I guess it's not crazy to imagine we might want the option to write images someday. But it will need some kind of unit test until then, since right now this function doesn't seem to be called by anything.
There was a problem hiding this comment.
Huh, I hadn't realized that. I think it'd probably be best to just remove it, then, and if we need it someday that will clarify what it should actually look like.
piff/readers.py
Outdated
| yield reader | ||
| return | ||
| else: | ||
| raise NotImplementedError(f"No reader for extension {ext!r}.") |
There was a problem hiding this comment.
I'm happy to drop the open method.
I like ABCs as a place to hang docstrings (most of the time derived-class methods can just inherit them) and more generally document the interface that users and implementers are expected to satisfy. But I'm happy to remove it if it's not the pattern you prefer, and just move the docstrings over to the FITS implementations.
piff/readers.py
Outdated
| elif isinstance(v, np.integer): | ||
| struct[k] = int(v) | ||
| elif isinstance(v, np.floating): | ||
| struct[k] = float(v) |
There was a problem hiding this comment.
I've been burned in the past by numpy scalar types not being exactly substitutable for their built-in counterparts, and have gotten in the habit of coding defensively around them. IIRC that's the only rationale for all but the bytes, case, which was necessary to have str round-trip through the FITS binary table. But let me see what happens if I drop some or all of these and get back to you with a more precise answer.
piff/writers.py
Outdated
| header = self._header | ||
| self._fits.write_image( | ||
| array, extname=self.get_full_name(name), header=self._header | ||
| ) |
There was a problem hiding this comment.
Huh, I hadn't realized that. I think it'd probably be best to just remove it, then, and if we need it someday that will clarify what it should actually look like.
TallJimbo
force-pushed
the
writers
branch
2 times, most recently
from
9fd3c9f
to
aa53486
Compare
This appears to be unnecessary; fitsio is already taking care that at least the kinds of strings we're using already round-trip.
|
I believe I've addressed all review comments, so this is ready for another pass. |
See #168 for motivation.
This refactors the current FITS read/write code to delegate to new
ReaderandWriterclasses that have FITS implementations, with a JSON-like alternate implementation envisioned but not implemented. The FITS implementations should write files that are readable by older versions of Piff (the only change to the outputs is that the Piff version is now written to all of the extension headers, not just some of them), and of course read files written by older versions of Piff.