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

Fix force_derivation bug #1627

Merged
merged 5 commits into from
Jun 9, 2022
Merged

Fix force_derivation bug #1627

merged 5 commits into from
Jun 9, 2022

Conversation

sloosvel
Copy link
Contributor

@sloosvel sloosvel commented Jun 8, 2022

Description

This PR adds a check that does not allow to combine force_derivation: false and wildcards in the timerange tag and should the previous force_derivation: false behaviour.

Closes #1626

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:

@sloosvel sloosvel added the bug Something isn't working label Jun 8, 2022
@sloosvel sloosvel added this to the v2.6.0 milestone Jun 8, 2022
@sloosvel sloosvel requested a review from schlunma June 8, 2022 15:58
@sloosvel
Copy link
Contributor Author

sloosvel commented Jun 8, 2022

Pending to improve the code coverage.

@codecov
Copy link

codecov bot commented Jun 8, 2022

Codecov Report

Merging #1627 (659b44a) into main (12f679a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1627   +/-   ##
=======================================
  Coverage   91.38%   91.38%           
=======================================
  Files         204      204           
  Lines       11172    11173    +1     
=======================================
+ Hits        10210    10211    +1     
  Misses        962      962           
Impacted Files Coverage Ξ”
esmvalcore/_recipe.py 95.91% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data
Powered by Codecov. Last update 12f679a...659b44a. Read the comment docs.

esmvalcore/_recipe.py Outdated Show resolved Hide resolved
Copy link
Contributor

@schlunma schlunma left a comment

Choose a reason for hiding this comment

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

Thanks for these quick changes Saskia, everything works fine now!!

One question: While testing this I realized that our data finder struggles to find data when the number of digits for the start year is longer than the number of digits of the end year (e.g. timerange: 200001/2001). The reason for this is that the comparison used to check if a file lies within the given timerange (start <= time_in_file <= end) never returns anything since the integer 200001 is never smaller than the integer 2001.

I don't think we can easily fix this in a general way, as padding these numbers automatically is quite dangerous (e.g., we also have 3-digit years, and can probably also have 5-digit years, in which cases an automatic padding would be plainly wrong).

I would suggest that we add a note to the doc that users need to be carefulhere and specify the same number of digits for the start and end period. Can I simply push this to this branch here? Thanks!!

@sloosvel
Copy link
Contributor Author

sloosvel commented Jun 9, 2022

I am not sure how this is related to this pull request, but if clarifying this is useful to you, please add the comments as needed.

@schlunma
Copy link
Contributor

schlunma commented Jun 9, 2022

The problem I encountered was that I got missing data errors when using timerange: */2000 for a dataset even though the data is there. Turns out the wildcard is resolved to 197901/2000 in this case, thus leading to the problem described above.

@sloosvel
Copy link
Contributor Author

sloosvel commented Jun 9, 2022

amazing, please push whatever is needed to the branch and approve it so it can be merged!

@sloosvel sloosvel merged commit 2698ac3 into main Jun 9, 2022
@sloosvel sloosvel deleted the dev_fix_derive_false branch June 9, 2022 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Derivation with force_derivation: false is broken
2 participants