As discussed I would like to introduce a second option of how b is calculated in the Holland 2010 model is calculated.
In this WIP pull request, could we discuss on the function descriptions and the naming?
I suggest to add a new function called _bs_holland_2010_v2 being another option to _bs_holland_2008 (also used in Holland 2010 in the currently implemented version of Holland 2010).
To select this function you would have to specify the parameter model='H10_v2' in TropCylcone.from_tracks.
What do you think about these names?

Also consider that we might add additional options at a later stage: e.g. how to calculate the static windfield (so an alternative function to _stat_holland_2010, again with another formula also mentioned in Holland 2010). This would then probably be selectable with model='H10_v3'

I just pushed my suggested alternative implementation (introducing a model_kwargs parameter).

In the future, this structure could also be used to implement the alternative static windfield formula from Holland et al. 2010 (mentioned in the OP), or to specify a second wind measurement (v_n, r_n).

I just took the opportunity to implement and push the alternative static windfield formula.

Edit: I first forgot to modify the computation of the exponent x to be consistent with the change of the static windfield formula. After fixing this, the results look completely fine.

Doing some more checks (trying to reproduce Fig. 1 from the original Holland et al. 2010 publication, and looking at the quite weak storm 2020034S13063), we found that the x-exponent needs truncation to prevent some unphysical results in both versions of the static windfield formula. I implemented these truncations in the latest commit.

@tovogt @ThomasRoosli What's the status here? I see that @tovogt reviewed the PR but also suggested an alternative implementation.

Should I review the current state? Note that due to time constraints, I will only be able to review the code itself, and not if it correctly implements the Holland model. I would rather that the two of you agree on that first.

I was able to double check the implementation with our TC expert Pamela Probst looking at many different storms. The Holland Model is correctly implemented. @peanutfun yes your help is greatly appreciated.

I'll review the code then as soon as possible. Thanks for the feedback!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating the Holland model. But I'm confused: Where's the second version mentioned in the PR?

The changes seem reasonable but they increased the size of an already huge module. Could we maybe split it into two files? From line 1132, there are only private methods. Could we move them into a separate module?

I have a few comments regarding the docstrings and tests. Apart from that, the implementation looks solid!

Thanks for updating the Holland model. But I'm confused: Where's the second version mentioned in the PR?

The changes seem reasonable but they increased the size of an already huge module. Could we maybe split it into two files? From line 1132, there are only private methods. Could we move them into a separate module?

I have a few comments regarding the docstrings and tests. Apart from that, the implementation looks solid!

Thanks @peanutfun for the review.
Regarding your larger questions:
We discontinued the structure with a "second version" proposed by me and replaced it with an "alternative approach" using model_kwargs proposed by tovogt.
I suggest splitting the module in two files in a seperate MR.
I tried to answer your comments below please have a look and resolve if you are satisfied.

@ThomasRoosli One open discussion. Once resolved, we can merge. Sorry for taking so long 🙇

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice work, @tovogt @ThomasRoosli!

Thanks for the latest addition. I'll add a changelog message and merge then.