-
Notifications
You must be signed in to change notification settings - Fork 168
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: change extrapolation values for resampling outside radar domain to zero #419
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #419 +/- ##
=======================================
Coverage 83.88% 83.88%
=======================================
Files 160 160
Lines 12780 12780
=======================================
Hits 10720 10720
Misses 2060 2060
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@ladc, what are your thoughts here? |
I also tried to fix this issue in the commit c7b7e0c (when addressing issue #370) but in a more complicated way. I changed the resampling to allow nan values in one of the arrays, and disregard the corresponding pixels in the resampling procedure. The results are different for the control member, but I cannot see a difference for a random member. Here's the animation comparing 447260c (first, Ruben's fix applied to my control member branch) and 7ef8a32 (last, my control member branch with modifications to resample_distributions and nonparam_match_empirical_cdf to allow nans). |
@ladc, I agree that your implementation would even be cleaner. If you're confident about it, could you push those resampling changes to this branch? |
Hi everyone, |
Are we closing this in favour of #428? |
That's OK for me. |
Fix for issue #416.
The small fix introduced here, which sets all extrapolation component values to zero outside the radar domain (instead of to the NWP value as in the code now), gives the following results for the case indicated in #390:
Radar observations (up to 60 minutes ahead):
Blended forecast for ensemble member 0, without the smoothing mask as introduced in #379:
Blended forecast for ensemble member 0, with the smoothing mask:
See also #390 for the previous results. The first lead time is still not perfect, but we got a lot closer already. What do you think?