Fix exponent clipping in Holland 2010 implementation #793
Conversation
peanutfun
left a comment
There was a problem hiding this comment.
The actual change looks easy enough, but it's really hard for me to understand the tests. Could we maybe reduce the number of values we are testing against? 🤔
| [0.5, 0.500000, 0.472730], | ||
| [0.5, 0.000000, 0.211602], | ||
| [0.5, 0.519094, 0.625478], | ||
| [0.5, 0.495423, 0.049174], | ||
| [0.5, 0.464344, 0.000000], |
There was a problem hiding this comment.
I am guessing the two zeros here are an effect of the clipping an something you wanted to explicitly test for?
There was a problem hiding this comment.
In my understanding, the first zero results from the large distance between track and centroids (close_centr = False), thus does not test the clipping. The last zero tests the clipping by creating the conflict v_max_s < v_n (v_max_s = 12 and v_n defaulting to 17). To me, it is also unclear what is tested in the other two, added cases.
|
Thanks for checking! I modified the test cases to be more systematic, and also added explanations in the code. |
|
Thanks for adapting the test. Your explanations make it is very clear what is tested. In my opinion, ready to merge. |
Changes proposed in this PR:
hol_xexponent to the [0.0, 0.5] interval, even though this is not mentioned in the literature. I understand that negative values are undesirable because it would cause the wind speeds to increase outwards (which is unphysical). However, I don't think that clipping high values to 0.5 is necessary. That would just mean that wind speeds are decreasing very fast outwards, which seems okay to me.PR Author Checklist
develop)PR Reviewer Checklist