Conversation
… and Holland 2010 model
…_vtrans # Conflicts: # CHANGELOG.md
tovogt
left a comment
tovogt
left a comment
There was a problem hiding this comment.
Thanks, great work! Ready to be merged from my side.
peanutfun
left a comment
peanutfun
left a comment
There was a problem hiding this comment.
I can't clearly say if the new implementation is correct. For the newly added model cases, the v_trans_corr seems to be applied twice.
The function is generally hard to read because of all the manual broadcasting and indexing. I think it would help to transpose the arrays, so that the default broadcasting rules of numpy apply (last dimensions are broadcast). But I think this would be too much for this PR. I have a few suggestions, though.
climada/hazard/trop_cyclone.py
Outdated
| # In these models, v_ang_norm already contains vtrans_norm, so subtract it first, before | ||
| # converting to vectors and then adding (vectorial) vtrans again. Make sure to apply the | ||
| # "absorbing factor" in both steps: | ||
| vtrans_norm_bc = np.broadcast_arrays(si_track["vtrans_norm"].values[:, None], d_centr)[0] |
There was a problem hiding this comment.
Make clear that we only want the broadcast of one array
| vtrans_norm_bc = np.broadcast_arrays(si_track["vtrans_norm"].values[:, None], d_centr)[0] | |
| vtrans_norm_bc = np.broadcast_to(si_track["vtrans_norm"].values[:, None], d_centr.shape) |
There was a problem hiding this comment.
It's true that it does the same. If you single out this line of code, it might be clearer the way you put it. But np.broadcast_arrays is used in a lot of places in this file, and I find it easier to read the whole code if similar code patterns are used everywhere. However, I don't really care that much, actually.
Edit: Seems like we are going to replace all instances of broadcast_arrays with broadcast_to now, which is totally fine with me.
I don't think that it is possible to reduce everything to numpy's default broadcasting rules because sometimes broadcasting is done in time, and sometimes in space (and sometimes both), and sometimes it's done in the two vectorial components of a vector field. In general, I think that code with manual broadcasting is better readable and safer because broadcasting is explicit and you (mostly) know what the code is doing. Implicit broadcasting can be dangerous... |
I would suggest to change broadcast_arrays to broadcast_to in all lines of the module (as suggested by @peanutfun ) and keep the explicit broadcasting (keep [:,:,::-1] instead of [...,::-1] ) for readability and safety. Does everyone agree, @tovogt and @peanutfun ? |
|
Fyi, I just tested what replacing |
Co-authored-by: Thomas Vogt <[email protected]>
Co-authored-by: Thomas Vogt <[email protected]>
peanutfun
left a comment
peanutfun
left a comment
There was a problem hiding this comment.
Very good, thank you for your additional contribution @tovogt. As you and @ThomasRoosli agree on the matter of "explicit indexing", we will leave it in. Great work, everyone! I will merge.
3 participants
… and Holland 2010 model
Changes proposed in this PR:
To highlight the advantage of removing the double counting. The calculated maximum windspeed is compared with the reported windspeeds for each timestep of 20 years of storm tracks data from ibtracks.
The comparison is done for the current develop branch and for the proposed changes. The proposed changes are able to (i) increase r-squared value and (ii) to decrease the intercept of a linear regression on this comparison. (results can be found here: https://github.com/ThomasRoosli/CLIMADA_test_TC_vtrans_addition/blob/main/CLIMADA_TC_holland_vtrans_comparison.ipynb).
This comes comparison was done additionally to a detailed investigation of several storms in the North Atlantic Ocean.
PR Author Checklist
develop)PR Reviewer Checklist