Skip to content
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

Merged
merged 4 commits into from
Oct 11, 2024

Conversation

LTDakin
Copy link
Contributor

@LTDakin LTDakin commented Oct 9, 2024

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 the sci_data, hdu, and fits file
  • FITSOutputHandler
    takes a cache key ndarray of data and contains methods to prepare the output and create it

Other Changes

  • Changed the operations to use the new classes
  • Operations log the output directly to save a cache call which will save some resources and speed up the operation
  • Changed mock functions to point to right place
  • Updated the test file since comments were changed

Copy link
Contributor

@jnation3406 jnation3406 left a 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...

datalab/datalab_session/data_operations/median.py Outdated Show resolved Hide resolved
datalab/datalab_session/data_operations/median.py Outdated Show resolved Hide resolved
Copy link
Contributor

@mgdaily mgdaily left a 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

@mgdaily
Copy link
Contributor

mgdaily commented Oct 10, 2024

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...

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.

Copy link
Contributor

@mgdaily mgdaily left a 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):
Copy link
Contributor

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.

@LTDakin LTDakin merged commit a9f5658 into main Oct 11, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants