-
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
Feature/output types #77
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.
Looks good but some questions to think about to make things more clear
@@ -43,7 +43,7 @@ def set_comment(self, comment: str): | |||
"""Add a comment to the FITS file.""" | |||
self.primary_hdu.header.add_comment(comment) | |||
|
|||
def create_and_save_data_products(self, index: int=None, large_jpg_path: str=None, small_jpg_path: str=None, tif_path: str=None): | |||
def create_and_save_data_products(self, format, index: int=None, large_jpg_path: str=None, small_jpg_path: str=None, tif_path: 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.
Doesn't this always create a fits file? So it doesn't need the format argument because its always fits if you call this function right?
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.
Hmm maybe a better solution in the long run is to create a smarter method of generating the outputs that can be flexible to all supported file types. For RGB we skip this step and create just the jpgs, but it would be more consistent to have one function for all the operations.
@@ -147,7 +147,7 @@ def get_fits(basename: str, source: str = 'archive'): | |||
finally: | |||
Path(fits_path).unlink(missing_ok=True) | |||
|
|||
def save_files_to_s3(cache_key, file_paths: dict, index=None): | |||
def save_files_to_s3(cache_key, format, file_paths: dict, index=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.
It feels a little weird choosing a format here when the file_paths also dictate a format. I think maybe theres just two things happening in this method now - submitting a list of files to S3, and creating the output dict. Maybe separating those could make it more clear? Or maybe this is just fine
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.
maybe the name could use some improvement but the function calls another helper add_file_to_bucket
that does the actually uploading. So the main purpose of this function is to structure your output for the frontend. I'm not sure how to break it down, but open to suggestions
This PR supports the frontend PR that lets us now filter inputs based on file type.
Added a Format class to keep track of supported datalab inputs and outputs. This will let us add new types as they come up in the future.
Changed the 'file' type for input descriptions to 'fits' to represent a working fits file.
RGB stack now has the output of type 'image' so that when you try to perform a median or other operation that requires proper fits files, RGB's output won't show up.