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

Add GOES Data Download Manager Script #240

Merged

Conversation

14Richa
Copy link
Contributor

@14Richa 14Richa commented Mar 24, 2024

Pull Request

Description

This pull request adds a script for downloading GOES data. The script uses the GOES2GO library to download data for a specified time range and product. It includes functionality to save the downloaded data to a specified directory and log any errors encountered during the download process.

Fixes #222

Checklist:

  • My code follows OCF's coding style guidelines
  • I have performed a self-review of my own code
  • I have checked my code and corrected any misspellings

Copy link
Member

@jacobbieker jacobbieker 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 a good start! Thanks for this work. Its getting closer to what I was thinking for the image downloading. Mostly just some comments related to what was mentioned in #222, and some more simple checks to speed up the downloading and not download the same data multiple times.

satip/goes_download_manager.py Show resolved Hide resolved
satip/goes_download_manager.py Outdated Show resolved Hide resolved
satip/goes_download_manager.py Outdated Show resolved Hide resolved
satip/goes_download_manager.py Outdated Show resolved Hide resolved
satip/goes_download_manager.py Show resolved Hide resolved
@14Richa
Copy link
Contributor Author

14Richa commented Mar 26, 2024

This is a good start! Thanks for this work. Its getting closer to what I was thinking for the image downloading. Mostly just some comments related to what was mentioned in #222, and some more simple checks to speed up the downloading and not download the same data multiple times.

Thanks for the comments @jacobbieker! I've addressed most of them and incorporated your suggestions. I've created a main DownloadManager class combining functionalities for EUMETSAT and GOES downloads. Also, I'm thinking of changing the filename from eumetsat.py to download_manager.py for better clarity. What do you think? Do you have any other suggestions for the filename?

Copy link
Member

@jacobbieker jacobbieker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvements! This kind of design is good, especially for being able to add more providers in the future. There will need to be some more changes to the code, as DownloadManager now doesn't do what it did before. So if you could update the other calls to be the EUMETSATDownloadManager that would be great! And might be the last changes for this PR I think.

satip/eumetsat.py Outdated Show resolved Hide resolved
@jacobbieker
Copy link
Member

This is a good start! Thanks for this work. Its getting closer to what I was thinking for the image downloading. Mostly just some comments related to what was mentioned in #222, and some more simple checks to speed up the downloading and not download the same data multiple times.

Thanks for the comments @jacobbieker! I've addressed most of them and incorporated your suggestions. I've created a main DownloadManager class combining functionalities for EUMETSAT and GOES downloads. Also, I'm thinking of changing the filename from eumetsat.py to download_manager.py for better clarity. What do you think? Do you have any other suggestions for the filename?

Yeah, making it download_manager.py is better, and moving the EUMETSATDownloadManager to a eumetsat.py file.

@14Richa
Copy link
Contributor Author

14Richa commented Mar 26, 2024

Nice improvements! This kind of design is good, especially for being able to add more providers in the future. There will need to be some more changes to the code, as DownloadManager now doesn't do what it did before. So if you could update the other calls to be the EUMETSATDownloadManager that would be great! And might be the last changes for this PR I think.

Thanks for the feedback @jacobbieker ! I have updated the code as per your suggestion, changing the other calls to use EUMETSATDownloadManager.

Copy link
Member

@jacobbieker jacobbieker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, one quick question left, then this I think is good to get merged! Did all the tests pass when you ran it?

satip/eumetsat.py Show resolved Hide resolved
@jacobbieker
Copy link
Member

Also, there will need to be a few updates to the tests, as they have changed in #229

@14Richa
Copy link
Contributor Author

14Richa commented Mar 27, 2024

Hey @jacobbieker, both of my tests are running successfully. Can we merge this PR now?

@jacobbieker jacobbieker self-requested a review March 27, 2024 13:27
Copy link
Member

@jacobbieker jacobbieker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks for this!

@jacobbieker jacobbieker merged commit 54fe9ec into openclimatefix:main Mar 27, 2024
2 checks passed
@peterdudfield
Copy link
Collaborator

@all-contributors please add @14Richa for code

Copy link
Contributor

@peterdudfield

I've put up a pull request to add @14Richa! 🎉

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.

Add support for GOES and Himawari Satellite Imagery
3 participants