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

Replay Mover #5

Merged
merged 17 commits into from
Dec 29, 2023
Merged

Replay Mover #5

merged 17 commits into from
Dec 29, 2023

Conversation

timothyas
Copy link
Member

This adds example code to move the 1 degree and 1/4 degree replay datasets. The README.md file contains an overview of what each file does, some notes on performance including what did not work for moving 30 years of 1/4 degree data, and notes on ensuring that access to GCS is secured.

I'll leave this as a draft for now and wait until the 1/4 degree jobs are done to merge. I'll tag people when that happens.

@timothyas timothyas marked this pull request as draft December 7, 2023 23:25
@timothyas timothyas force-pushed the feature/replay-mover branch from e386e73 to 397875a Compare December 11, 2023 17:09
@timothyas timothyas marked this pull request as ready for review December 21, 2023 20:50
@timothyas timothyas requested a review from frolovsa December 21, 2023 20:50
@timothyas
Copy link
Member Author

@danielabdi-noaa could you give this a look and make sure it looks OK to you?

@frolovsa it's ready for you to take a look as well whenever you get the chance

Copy link

@frolovsa frolovsa left a comment

Choose a reason for hiding this comment

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

@timothyas thank you for your contribution. Overall this is very clear. As we discussed in-person. Lets make an issue capturing the idea that some of the hard coded values should be abstracted so the .py code can be reused without editing.

Copy link
Contributor

@danielabdi-noaa danielabdi-noaa left a comment

Choose a reason for hiding this comment

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

@timothyas This looks great to me! I have left a few suggestions mostly because I have to :) I understand these are one-off scripts so my suggestions to make them more generic are probably not worth the time. I know what to edit to get it running for my case after our meeting the other day. Thanks!

ufs2arco/timer.py Outdated Show resolved Hide resolved
ufs2arco/timer.py Outdated Show resolved Hide resolved
ufs2arco/timer.py Outdated Show resolved Hide resolved
examples/replay/replay_mover.py Outdated Show resolved Hide resolved
examples/replay/fill_quarter_degree.py Show resolved Hide resolved
examples/replay/fill_quarter_degree.py Show resolved Hide resolved
examples/replay/replay_filler.py Show resolved Hide resolved
@timothyas
Copy link
Member Author

Thanks @frolovsa and @danielabdi-noaa for all of the comments! I definitely agree that we can generalize this. @frolovsa And I talked about merging this without generalizing the behavior as a first pass. I'll raise an issue and we can all agree on the generalizations, and we can implement that in the future. Does that sound good @danielabdi-noaa ? Other than that I'll address some of your detailed comments (thanks for those!) next week when I'm in front of a computer before merging.

@danielabdi-noaa
Copy link
Contributor

@timothyas Yes, that sounds good to me. Happy holidays!

@timothyas
Copy link
Member Author

You too @danielabdi-noaa!

@timothyas
Copy link
Member Author

@danielabdi-noaa and @frolovsa I addressed your comments either by mentioning the points of generalization in #12 or in the most recent commits. Let me know if you think it's good to go and I'll merge. Thanks again!

@danielabdi-noaa
Copy link
Contributor

@timothyas Looks good to me! Thanks for addressing my suggestions.

Copy link

@frolovsa frolovsa left a comment

Choose a reason for hiding this comment

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

let merge this thing!
Thankyou for your hard work Tim and Dan

@timothyas timothyas merged commit c489666 into NOAA-PSL:main Dec 29, 2023
3 checks passed
@timothyas timothyas deleted the feature/replay-mover branch December 29, 2023 01:00
@timothyas timothyas mentioned this pull request Mar 18, 2024
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