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 tag all_years: True #1122

Closed
wants to merge 25 commits into from
Closed

Add tag all_years: True #1122

wants to merge 25 commits into from

Conversation

sloosvel
Copy link
Contributor

@sloosvel sloosvel commented May 11, 2021

Description

This pull request adds the option to load all years in an experiment. To be merged after #771, as I started from a commit in there because the changes were already there. I will update the branch after the merge of #771 to make these changes more clear.

Closes #1120

Link to documentation:


Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.


To help with the number pull requests:

@valeriupredoi
Copy link
Contributor

I think a more user-friendly (and introducing less overhead with less optional arguments) is to leave out either start_year or end_year or both - no start_year means the code finds all the data before end_year, no end_year means it finds all the data after start_year and neither start nor end years meaning ALL the data available. Bit of a headache to implement but much nicer and with more scope IMHO 🍺

@zklaus
Copy link

zklaus commented May 11, 2021

I think a more user-friendly (and introducing less overhead with less optional arguments) is to leave out either start_year or end_year or both - no start_year means the code finds all the data before end_year, no end_year means it finds all the data after start_year and neither start nor end years meaning ALL the data available. Bit of a headache to implement but much nicer and with more scope IMHO beer

I am not a fan of this implicit approach. It makes the recipe depend on the available data and thus reduces the reproducibility. But I think there is an opportunity here: Other fairly common requests are things like using the first/last 20 years of an experiment (useful for monitoring among other things). Could we have something like

timeselection_mode: normal/first 20yr/last 20yr/all_experiment/all_available

?

Not sure about the exact API here. I know this is urgent for @sloosvel and @jvegasbsc, so I would also be ok with something simpler now, but if you can come up with something easy and generic, that would be great.

@sloosvel
Copy link
Contributor Author

I think @zklaus idea is more in line with the work that is already done, because the pull request already finds all data available and it would just be a matter of subsetting it.

@valeriupredoi
Copy link
Contributor

@zklaus @sloosvel sounds good to me! Sorry, I didn't realize this was urgent and don't want to sandbag it. The implicit configuration is useful (maybe not so implicit) since I've heard a lot of them scientists making noises they want to use all the available data on a particular site, they're not concerned how much it is available, but rather they just want to grab it all, and am honestly not a big fan of more configuration arguments/keys/definitions. But let's discuss this in the future, after this PR has been merged - Saskia, I can review it now if you ready!

@sloosvel
Copy link
Contributor Author

Let me give it a go at the timeselection range, and if it's too much trouble maybe we can do with this?

@sloosvel
Copy link
Contributor Author

I'm closing this and opening a new PR.

@sloosvel sloosvel closed this May 13, 2021
@sloosvel sloosvel deleted the dev_load_all_years branch March 15, 2022 14:11
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 option to load selected or all years available in an experiment
3 participants