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 2-weekly slicing for 2022 #3

Merged
merged 6 commits into from
Jul 24, 2024
Merged

Add 2-weekly slicing for 2022 #3

merged 6 commits into from
Jul 24, 2024

Conversation

phinate
Copy link
Collaborator

@phinate phinate commented Jul 17, 2024

No description provided.

@phinate phinate requested review from IFenton and dfulu and removed request for IFenton July 17, 2024 14:17
@phinate
Copy link
Collaborator Author

phinate commented Jul 17, 2024

We should definitely add some tests for this (and the other download functionality) properly to make sure the slicing makes sense. For now I'm happy if a couple people can try downloading it and get the expected result! Maybe choose a large value for data_inner_steps :)

@phinate phinate requested review from IFenton and removed request for dfulu July 17, 2024 14:18
Copy link
Collaborator

@dfulu dfulu left a comment

Choose a reason for hiding this comment

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

Thanks @phinate, looks really great. The only concern I have is that minor 2 week split comment

src/cloudcasting/download.py Outdated Show resolved Hide resolved
Co-authored-by: Isabel Fenton <[email protected]>
@phinate phinate force-pushed the validation-filtering branch from 239ec81 to be2bf48 Compare July 19, 2024 16:32
Copy link
Collaborator

@IFenton IFenton left a comment

Choose a reason for hiding this comment

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

LGTM. I tried running a few examples and it seems it will work as expected once we change to dayofyear.

It might also be worth adding a message (or maybe an error) if you try to run the valid flag for any year except 2022. Otherwise you might think you'll download two separate identical datasets and think they are different

@phinate
Copy link
Collaborator Author

phinate commented Jul 24, 2024

Okay, I've implemented the fix we discussed: we now use a date range as suggested by @dfulu. Please take a look and approve if you're happy! :)

@phinate phinate requested review from IFenton and dfulu July 24, 2024 10:30
src/cloudcasting/download.py Outdated Show resolved Hide resolved
@dfulu
Copy link
Collaborator

dfulu commented Jul 24, 2024

@phinate, this looks really good. I've just got one final comment

Copy link
Collaborator

@dfulu dfulu left a comment

Choose a reason for hiding this comment

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

Looks great!

@phinate phinate merged commit cc3af87 into main Jul 24, 2024
3 checks passed
@phinate phinate deleted the validation-filtering branch July 24, 2024 14:04
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