Skip to content
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

Fix exponent clipping in Holland 2010 implementation #793

Merged
merged 3 commits into from
Nov 7, 2023

Conversation

tovogt
Copy link
Collaborator

@tovogt tovogt commented Oct 18, 2023

Changes proposed in this PR:

  • This PR is in response to a mail by a PhD student who had questions about the implementation of the Holland 2010 TC wind model in CLIMADA.
  • The current implementation clips the hol_x exponent 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.
  • While nobody currently seems to use the Holland 2010 wind model (including me), it might still be interesting to ensure the correctness of the implementation.

PR Author Checklist

PR Reviewer Checklist

Copy link
Member

@peanutfun peanutfun left a comment

Choose a reason for hiding this comment

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

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

Comment on lines 329 to 333
[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],
Copy link
Member

Choose a reason for hiding this comment

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

I am guessing the two zeros here are an effect of the clipping an something you wanted to explicitly test for?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

@tovogt
Copy link
Collaborator Author

tovogt commented Nov 6, 2023

Thanks for checking!

I modified the test cases to be more systematic, and also added explanations in the code.

@leonie-villiger
Copy link
Collaborator

Thanks for adapting the test. Your explanations make it is very clear what is tested. In my opinion, ready to merge.

Copy link
Member

@peanutfun peanutfun left a comment

Choose a reason for hiding this comment

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

This is much clearer now. Thank you, @tovogt! Merging.

@peanutfun peanutfun merged commit 8c8ee96 into develop Nov 7, 2023
11 checks passed
@emanuel-schmid emanuel-schmid deleted the feature/fix_holland_2010 branch November 8, 2023 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants