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.

Sign up for GitHub

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

Jump to bottom

PresRat correction #215

Merged
merged 189 commits into from
Jul 23, 2024
Merged

PresRat correction #215

merged 189 commits into from
Jul 23, 2024

Conversation

castelao
Copy link
Member

@castelao castelao commented May 6, 2024

Implementing PresRat for precipitation.

@castelao castelao requested review from grantbuster and bnb32 May 6, 2024 20:30
@castelao castelao self-assigned this May 6, 2024
@castelao castelao force-pushed the Gui/PresRat branch 2 times, most recently from 3106f0b to 310d280 Compare May 15, 2024 15:46
castelao added a commit that referenced this pull request May 20, 2024
@castelao
Combining ruff setup from PR #215
9456dd3 
To keep the following PR cleaner, this commit moves and combines a few
ruff setup from that PR into here.

style: Adding 'F' into ruff's checks

  It seems to be the only one conforming.

cfg: Missing to ignore F401

  Following @bnb32 setup.

style: Ignore F401 only on __init__

  Thanks to @ppinchuk 's tip! I missed that before.

Ignoring unused import in docs

Including Warning (W) rules
@castelao castelao force-pushed the Gui/PresRat branch 2 times, most recently from d916ea9 to 8823368 Compare June 24, 2024 19:54
@castelao castelao marked this pull request as ready for review June 25, 2024 14:43
bnb32
bnb32 requested changes Jun 26, 2024
View reviewed changes
Copy link
Collaborator

@bnb32 bnb32 left a comment

Choose a reason for hiding this comment

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

I don't know the details of the presrat implementation but the mechanics here look good to me. Just a couple tweaks. Also, let's add a fwp integration test like with qdm.

sup3r/bias/bias_transforms.py Outdated Show resolved Hide resolved
sup3r/bias/qdm.py Outdated Show resolved Hide resolved
sup3r/bias/qdm.py Show resolved Hide resolved
@castelao castelao mentioned this pull request Jun 27, 2024
Merged
@castelao castelao force-pushed the Gui/PresRat branch 2 times, most recently from f4e53bb to 8a5d7b7 Compare July 1, 2024 20:58
@castelao castelao mentioned this pull request Jul 2, 2024
Merged
@castelao castelao marked this pull request as draft July 8, 2024 17:32
grantbuster
grantbuster requested changes Jul 10, 2024
View reviewed changes
sup3r/bias/bias_transforms.py Outdated Show resolved Hide resolved
sup3r/bias/bias_transforms.py Outdated Show resolved Hide resolved
sup3r/bias/bias_transforms.py Outdated Show resolved Hide resolved
sup3r/bias/bias_transforms.py Outdated Show resolved Hide resolved
sup3r/bias/bias_transforms.py Outdated Show resolved Hide resolved
sup3r/bias/qdm.py Outdated Show resolved Hide resolved
sup3r/bias/bias_transforms.py Outdated Show resolved Hide resolved
sup3r/bias/bias_transforms.py Outdated Show resolved Hide resolved
sup3r/bias/qdm.py Outdated Show resolved Hide resolved
tests/bias/test_presrat_bias_correction.py Show resolved Hide resolved
grantbuster
grantbuster reviewed Jul 10, 2024
View reviewed changes
sup3r/bias/qdm.py Outdated Show resolved Hide resolved
This was referenced Jul 15, 2024
Merged
Open
@castelao
Copy link
Member Author

To keep everyone in the same page. This PR is still missing a couple of tests and a cleanup. I'll remove the draft state when it's ready.

@castelao
Copy link
Member Author

A reminder for myself. A small edition in the documentation, as suggested by @grantbuster:
"Okay i think I see your point about how this will actually totally work with hourly data because DOY is being used in the indexing. Maybe note in the _window_center docstring that the output array indexes the day of year? Looks good to me! I'm not worried about fractional days for leap years."

@castelao
test, fix: Fixed test_presrat_transform_nochanges()
9b391f5 
It was missing to force K=1, otherwise any difference in the means would
result in a K that would change the unbiased.
@castelao
refact: Removing apply_zero_precipitation_rate()
0a78b31 
Removing apply_zero_precipitation_rate() as requested by @grantbuster
@castelao
refact: PresRat with no_trend reduces to ordinary QDM
3459e02 
If no_trend is True, skip zero rate and k-factor corrections.

As requested by @grantbuster
@grantbuster
Copy link
Member

@castelao, i took another look and the implementation looks great. I know you're adding a presrat-motivation test (e.g., testing the reason why we're doing presrat) which should be great. Could you also clean up docstrings in bias_transforms.py and inline comments in qdm.py? Specifically, there are just some docstrings in bias_transforms.py that have arguments out-of-order or are missing returns statements and lots of methods/todo notes in qdm.py that should be cleaned up.

There is also one outstanding comment on assert data.ndim in bias_transforms.py

@castelao castelao marked this pull request as ready for review July 23, 2024 04:33
@castelao
cfg: Updating pixi.lock
7394427
@castelao
fix: Removing method zero_rate_threshold
0b81c67 
I missed this method from an early stage implementation. Thanks to
@grantbuster who noticed that!

Also defining default `zero_rate_threshold` to zero as requested by
Grant.
@castelao
fix: zero_precipitation_rate()
95f2e00 
The rate should be <= otherwise we would miss completely the case of a
threshold equal to zero.

Thanks to @grantbuster who noticed that.
@castelao
doc: A more meaningfull example for write_outputs()
b5ab7f9
@castelao castelao merged commit 1431d2f into main Jul 23, 2024
12 checks passed
@castelao castelao deleted the Gui/PresRat branch July 23, 2024 18:42
github-actions bot pushed a commit that referenced this pull request Jul 23, 2024
@castelao
Merge pull request #215 from NREL/Gui/PresRat
1d712b0 
PresRat correction
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnb32 bnb32 bnb32 approved these changes

@grantbuster grantbuster grantbuster approved these changes

Assignees

@castelao castelao

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@castelao @grantbuster @bnb32