-
Notifications
You must be signed in to change notification settings - Fork 169
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
Refactor the blending/steps.py nowcasting code #395
Comments
Yes please! |
@RubenImhoff I went trough the whole code to see what was unclear for me. I will already paste my findings here and maybe we can discuss later today. Ill first time some time to eat :p Specific lines of needed name changes Line 583: R_f should get a new name Line 600: R_f_ empty_precip? Line 616: outarr Line 677: pp, generate_noise, noise_std_coeffs = _init_noise( Line 716: randgen_prec, vps, generate_vel_noise = _init_random_generators( Line 727: D, D_Yn, D_pb, R_f, R_m, mask_rim, struct, fft_objs = _prepare_forecast_loop( Line 784: forecast_prev = precip_cascade Line 790: This timestep function is extremly complicated to understand, maybe changing some names would make this more clear Line 858 rho_extr would change to rho_extrap (just a bit more clear what this is) Line 865 R_f_ should be renamed: R_f_temp Line 922: cov -> covariance_radar_model? Line 955: EPS should be changed, quite unclear what it is Line 997: EPS_ should be changed Line 1028: R_f_ip Line 1033: Yn_ip Line 1051: t_diff_prev Line 1065: V_stack Line 1073: V_model_ Line 1094: R_f_im_recomp Line 1111: R_f_ep_recomp_ Line 1118: R_f_ep_recomp Line 1119: temp_mask Line 1122: R_f_ep Line 1144: Yn_ip_recomp Line 1158: Yn_ep_recomp Line 1159: Yn_ep Line 1173: R_f_ep_out, Yn_ep_out, R_pm_ep Line 1285: R_f_out Line 1339: cascades_stacked_ Line 1356: R_f_blended Line 1361: R_f_blended_mod_only Line 1384: R_f_new Line 1391: R_f_new_mod_only Line 1412: weights_pm, weights_pm_normalized, weights_pm_mod_only, Line 1438 Better commenting? Line 1439: R_pm_blended Line 1447: R_pm_blended_mod_only Line 1490: Can be removed line 1517: R_cmin Line 1525: MASK_prec -> MASK_precip or MASK_precip_cell? Line 1550: mu_0, MASK, mu_fct Line 1578: R_f_stacked General: We should look at all these t_... variables, they are quite strange Do we go for mu and sigma or means and sigmas? Should be uniform choice R_f = precip_forecast? is this correct? |
For all of these, I could not find the names in the nowcast steps but it is really hard to see the variables through the forest :) . I also tried to do this matching by using some generative AI tools but it does not preform well on this sort of tasks |
To give a brief answer (already): General: We should look at all these t_... variables, they are quite strange --> Fair, if doable :) Do we go for mu and sigma or means and sigmas? Should be uniform choice --> I would go for means and sigmas then :) R_f = precip_forecast? is this correct? --> Correct |
short comment: when there are so many variables given and returned by functions, it's usually a good indication that you need a class. so why not refactoring the code into a class and so that the methods can simply access attributes instead of having to pass them around all the time? the main interface can stay the same, that is, |
I agree this is the most elegant solution but maybe we should tackle that problem after name changes and changing the monolith into something more readable? |
My work today so the refactoring can be done in the future; things like splitting up the code into more modular functions or making a class can be added after the current steps I list below: STEP 1:
STEP 3: STEP 4: Please provide me with some feedback. |
I just went trough the code to try and refactor it. A version This is just a proof of concept is now pushed to try and show my suggestions (however with the old names as I did not push my version with name changes yesterday...) |
Refactoring Plan
|
@RubenImhoff @dnerini would now be a good time to refactor the code in steps blending and nowcasting? I have talked to @mats-knmi about how this would be integrated in the x-array version. This would be done after the changes are done in the current versions and once all tests are passed. |
sounds very good! |
@dnerini just to confirm if this would be okay. I was planning to refactor this code to a class due to all the variables. Does this conform to the pysteps design philosophy or should I just keep all the variables as local variables and remove the class based approach? |
@sidekock Go ahead with the refactoring. Yes, true, at the beginning we explicitly decided to avoid oop, but i believe the limits and shortcomings of that decision are quite clear.. so let's give it a go! |
@sidekock, sorry I was on vacation, so therefore my slow response. Great if you want to make a start with this! :) |
@sidekock, since I am planning on starting with the blending conversion to xarray as well soon, I would appreciate it if you could create a draft pull request from the branch you are currently working on and add me as (one of) the reviewers. This way I can easily keep track of your changes and take this into account when working on the xarray stuff. |
@mats-knmi Will do! I will probably only start somewhere later this week. The teaching season is once again upon me so first need to make sure I am ready on that front :) |
@mats-knmi I will start this afternoon with the re-factoring. Will start with the nowcast steps to have a template and then look at the blending steps. Ill add you as a reviewer as soon as the nowcast steps is refactored @RubenImhoff can I add you too? |
So I started today but I have a general question regarding the _check_inputs function. This is used to check some of the inputs put others are then checked in the forecast loop itself. Would it not be best to add all these checks and preparatory assignments (eg None -> dict()) in this _check_inputs? |
Of course! :) |
That may be a better way indeed. |
Start with low-hanging fruit:
The text was updated successfully, but these errors were encountered: