-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor write/read methods to delegate to new Writer/Reader interface. #170
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Jim. I have a few comments, but this is broadly what I had in mind.
piff/readers.py
Outdated
yield reader | ||
return | ||
else: | ||
raise NotImplementedError(f"No reader for extension {ext!r}.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added some replies and will go do a round of edits.
piff/readers.py
Outdated
yield reader | ||
return | ||
else: | ||
raise NotImplementedError(f"No reader for extension {ext!r}.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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
Reader
andWriter
classes 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.