Feature/tc holland vtrans

[ThomasRoosli]

… and Holland 2010 model

Changes proposed in this PR:

• in the Holland model 2008 and 2010 implementation a doublecounting of translational velocity is removed
• as the resulting v_ang_norm already contains vtrans_norm, we now subtract it first, before converting the angular velocity and the translational velocity to vectors and then adding (vectorial) vtrans to (vectorial (v_ang).

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

• Read the Contribution Guide
• Correct target branch selected (if unsure, select develop)
• Descriptive pull request title added
• Source branch up-to-date with target branch
• Documentation updated
• Tests updated
• Tests passing
• No new linter issues
• Changelog updated

PR Reviewer Checklist

• Read the Contribution Guide
• CLIMADA Reviewer Checklist passed
• Tests passing
• No new linter issues

[tovogt]

Thanks, great work! Ready to be merged from my side.

[peanutfun]

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.

[peanutfun]

Make clear that we only want the broadcast of one array

Suggested change

	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)

[tovogt]

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.

[tovogt]

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.

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...

[ThomasRoosli]

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.

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 always 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 ?

[tovogt]

Fyi, I just tested what replacing broadcast_arrays with broadcast_to would look like. I actually like it and decided to push it to your branch. Feel free to revert if you disagree.

[peanutfun]

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.