-
Notifications
You must be signed in to change notification settings - Fork 172
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
Allow for semi dynamic incremental mask in blending setup #459
base: master
Are you sure you want to change the base?
Conversation
To show what typically happens, after some time steps (often the longer lead times during the blending), the rainfall fields that predominantly follow from the noise generation tend to get cut off too strongly by the mask: With the suggested changes, this will change to (which I think looks more natural): By the way, the 'hard' edges in the north and east are the edge of the domain (this is zoomed in). |
Hi Ruben, |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #459 +/- ##
=======================================
Coverage 84.55% 84.55%
=======================================
Files 162 162
Lines 13458 13460 +2
=======================================
+ Hits 11379 11381 +2
Misses 2079 2079
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Yes, good suggestion, @sidekock. I went for the quick and dirty way (otherwise it becomes quite some work to plot this nicely including the final precip fields): The figures below show 1 = within mask and 0 = outside mask (so masked out in the post-processing). I've chosen a At first lead time (both masks should still be the same), left is proposed approach, right is 'old' approach: Lead time 10 (15-min time step, so after ~2.5 hours): Lead time 20 (so after ~5 hours): Lead time 52 (so after 13 hours): The rainfall fields (forecast) for these lead times were as follows: I still see some strong edge effect, @sidekock, while I'm using a |
I don't know a lot about this part of the code, but the proposed solution looks good to me. A simple way to achieve the goal. I agree that this results in more realistic looking precipitation fields. Is there a downside to this approach vs the other option or any other possible solution? |
@RubenImhoff, thanks for these plots. Could I ask for some more :)
Currently, I have no clue what is going on here. Maybe just a check but what is the data just pat that edge on timestep 52? are those nans? I assume so and that is indeed what my implementation is using. |
You mean the forecast rainfall fields, then the one of 40 The edge effects actually seem strongest at timestep 1, which is indeed where there are nans just at the edges of the domain. |
Indeed, I want to compare the 10 to the 40 on the actual rain values and not just the mask. I'll dive into the code to check what is going on with this mask. I don't fully know what is going on, but I will run some tests and plot the results to see what is going on... |
Not really a downside, I think. It increases computation time slightly and the buffer cannot grow indefinitely (but maybe that is okay?), because the |
This is for As you can see, the differences with 40 are small, but become visible for the smaller rainfall fields/cells that pop up. So especially for convective days, this will make a difference (and that is where I also used to see the cut off that was too strict). |
Thanks a lot for your work on this, I was also looking for the source of these undesirable "diamonds" and a possible solution. The effect was especially noticeable on the probability plots. Did you have a look at these in the new version? |
@RubenImhoff would it be possible to give me the exact configuration of you blended pysteps run? That makes it easyer to run some tests |
PS: actually the maps you gave sent are clear enough and probably the same pattern can be expected in the probability maps |
@RubenImhoff I took another look at the plots you sent over and I think the issue is just the data you are using. Correct me if I am wrong but as far as I can see, this is what I observe:
Hi @sidekock, you are right. That might be it then. If I see any other strange effect for later timesteps, I'll let you know. But this explains it for now, I think. |
Shall we continue with this method? Then I'll add some tests. :) |
why the question? is there any argument against it? :) |
None so far - just checking whether we agree to indeed go for it. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, Ruben! I'll just push a tiny simplification and run a test and get back to you.
Sorry, it seems I accidentally marked this ready for review. Anyway, I checked the output with a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lookin' good.
@RubenImhoff Is this ready to |
This PR fixes issue #381.
In the extrapolation based nowcasting, the mask is dilated iteratively at every timestep. This is easy to do in Lagrangian space for a pure extrapolation-based nowcast. However, in the case of STEPS blending with NWP, this is less straightforward because we are no longer in Lagrangian space (NWP is not advected). Right now, it's dilated only once but this does not grow further per timestep.
I've made a first start to make the incremental mask computation more dynamic (i.e., more incremental) over time. I've done so by including a
max_mask_rim
next to themask_rim
in themask_kwargs
. With increasing lead time, the mask_rim is then allowed to increase with the time step index on top of themask_rim
up to the point wheremax_mask_rim
is reached. This to ensure that we do not increase run times too much, while a very large mask_rim is not doing much as it gets cut off by the rainfall threshold at some point.Another option would be to make the
iterate_structure
setup below dependent on the time step:However, this should be stopped after a certain amount of lead times, as the
struct
parameter becomes way to big (and slow) otherwise.Or, if any of you has other suggestions, please let me know!
To do's: