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

Use max rain pixels for timestep and member to decompose for precip cascades in case of zero radar #461

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mats-knmi
Copy link
Contributor

This is something I noticed while working on the other PR. But I think this makes more sense than what I did during the memory reduction PR.

I used to look for a timestep and member that has any rain so that that can be used to decompose the model precip. But if we just use the timestep and member that has the most rainy pixels, this should be even better right? Since this was already computed a bit further down the line, this should be a simplification of the code without any significant side effects.

Let me know what you think.

Copy link

codecov bot commented Feb 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.23%. Comparing base (006fb49) to head (41b621a).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #461      +/-   ##
==========================================
- Coverage   84.24%   84.23%   -0.02%     
==========================================
  Files         160      160              
  Lines       13243    13234       -9     
==========================================
- Hits        11156    11147       -9     
  Misses       2087     2087              
Flag Coverage Δ
unit_tests 84.23% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dnerini
Copy link
Member

dnerini commented Feb 26, 2025

I used to look for a timestep and member that has any rain so that that can be used to decompose the model precip. But if we just use the timestep and member that has the most rainy pixels, this should be even better right?

Makes sense to me! Is there any equivalent logic in the radar-only nowcast method that could benefit from this too?

@sidekock
Copy link
Contributor

sidekock commented Feb 26, 2025

I agree, this is much cleaner.
Could you also change the following while you are changing the code @mats-knmi :

# Initialize noise based on the NWP field time step where the fraction of rainy cells is highest if self.__config.precip_threshold is None: self.__config.precip_threshold = np.nanmin(self.__precip_models)

Here we overwrite the config in the same function you are touching. However, StepsBlendingConfig is frozen so should not be written to. A new variable should be made in StepsBlendingParams and we should look where in the code after this point self.__config.precip_threshold is used, it should be changed for self.__params.precip_threshold

@mats-knmi
Copy link
Contributor Author

@dnerini As far as I know this is a blending only functionality as this uses the model data to create noise when the radar data has no precipitation. This is ofcourse not possible in a radar only nowcast.

@sidekock I added a commit. Is this what you meant?

@sidekock
Copy link
Contributor

sidekock commented Mar 3, 2025

@mats-knmi indeed, that is what I was aiming for. Weird the tests are faling on ".tests\test_plugins_support.py::test_importers_plugins - AssertionError: assert False"...

@mats-knmi
Copy link
Contributor Author

@sidekock This has nothing to do with this PR, apparently something with the cookiecutter plugin (see #460 (comment)).

@ladc
Copy link
Contributor

ladc commented Mar 4, 2025

Yes, our bad, sorry! Working on it.

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.

4 participants