-
Notifications
You must be signed in to change notification settings - Fork 0
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
fits_file_reader and output_handler classes, changes to operations, tests #38
Conversation
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 looks ok, but didn't Matt want to do this refactor to reduce the number of mocks? It looks like you just swapped out 3 mocks with 3 other mocks...
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 is looking better, but I still have a few bits of feedback. I think having docstrings documenting each of the methods in the helper classes would be good, their parameters, and their output. These are helper classes that a new DataLab contributor may use in the future.
Google docstrings are quite good: https://realpython.com/documenting-python-code/#google-docstrings-example
That wasn't the main motivation, but yes, the amount of mocking points to some additional restructuring that might need to occur. I had noticed that we had a lot of duplicated business logic (mostly written in a purely functional manner) for 1) retrieving data given a set of filenames, and 2) writing the necessary output to s3/returning it to the frontend. So I workshopped a more OO approach with Lloyd to create a couple of classes to encapsulate these and that could be used to aid the developer in getting the data we need and writing it out to the correct place within a DataOperation. |
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.
Looking a lot better! I have a couple of comments of things that could be further clarified.
with fits.open(self.fits_file) as hdul: | ||
return f"{self.basename}@{self.fits_file}\nHDU List\n{self.hdul.info()}" | ||
|
||
def get_hdu(self, extension: str=None): |
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 would recommend this be called get_hdu_by_extension_name, but I won't die on that hill.
Description
Moving away from using the util functions directly in a functional way to a class based approach which will allow for easier testing in the future and flexibility if we need to change parts of the operation or create operations that require different handling methods.
2 New Classes
FITSFileReader
takes a
basename
and allows for methods that get thesci_data
,hdu
, andfits file
FITSOutputHandler
takes a
cache key
ndarray of data and contains methods to prepare the output and create itOther Changes