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 post-downscaling wet day correction in the workflows #352

Merged

Conversation

emileten
Copy link
Contributor

@brews I went ahead and added changes to make use of the option developed here ClimateImpactLab/dodola#141. From the biascorrectdownscale template, I passed over the already existing correct-wetday-frequency parameter onto the qplad template, then within it to with-lat-chunks and within it to apply-qplad. At that point the value of the parameter is passed to the boolean click option --wet-day-post-correction. How does this sound to you ? I was thinking about testing that in an actual e2e-pr workflow run, but I wanted your input first.

Also -- I think that's for @dgergel -- note this means there isn't an option to apply this correction only pre or only post downscaling, it arbitrarily does both. Is that ok ?

@emileten emileten changed the title add post-downscaling wet day correction in the worfklow add post-downscaling wet day correction in the workflows Nov 26, 2021
Copy link
Member

@dgergel dgergel left a comment

Choose a reason for hiding this comment

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

@emileten it's fine if the pipeline only does both, as there isn't a situation in which we'd want to do either/or. We do want it only for precip, but I don't think you changed that so that's okay.

@brews would be great if you reviewed this too before we merge.

@brews brews self-requested a review November 30, 2021 01:05
@brews
Copy link
Member

brews commented Nov 30, 2021

Nice! @emileten This is going to need a few changes. I'm going to move this into a "draft" PR, until we walk through how to test and run the dev releases of dodola, @emileten, then we can review this.

@brews brews marked this pull request as draft November 30, 2021 01:08
@emileten emileten self-assigned this Nov 30, 2021
@emileten
Copy link
Contributor Author

@brews is this needed to run a "correct" e2e precip workflow?

@emileten
Copy link
Contributor Author

emileten commented Dec 21, 2021

@brews I think I fixed one problem with this, not sure it was all or even part of what you were worried about.

Back then I added the correct-wetday-frequency option to qplad.qplad. But not to qplad.main (entrypoint) which under the hood calls qplad.qplad. Without no default value specified anywhere, a qplad workflow running from its entrypoint would have crashed.

So, the few commits above I think should fix this.

@brews brews marked this pull request as ready for review December 21, 2021 01:35
Copy link
Member

@brews brews left a comment

Choose a reason for hiding this comment

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

Appreciate this contribution, @emileten ! I think this is good to go. 👍

@brews
Copy link
Member

brews commented Dec 28, 2021

I'm going to merge this and begin testing now that we've got some precipitation data to play with from #485.

@brews brews merged commit 621e9db into ClimateImpactLab:master Dec 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants