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

Shift cyclostrophic argument to model_kwargs in from_tracks #936

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

spjuhel
Copy link
Collaborator

@spjuhel spjuhel commented Aug 9, 2024

Changes proposed in this PR:

  • Makes "cyclostrophic" a model_kwarg in from_tracks
  • Adds documentation on this possibility in from_tracks
  • Removes the explicit argument in compute_angular_windspeeds and _compute_windfields such that it uses the value in model_kwarg.
  • Changes LOGGER.debug to LOGGER.warning in _compute_angular_windspeeds_h10 to inform user that cyclostrophic=False is ignored in this case

This last point is subject to debate, raising an error is also possible. The question then is, should it be raised rather in the top-level (i.e., in from_tracks) or low-level (i.e., in _compute_angular_windspeeds_h10)

--

This PR fixes #897

PR Author Checklist

PR Reviewer Checklist

@spjuhel spjuhel changed the base branch from main to develop August 9, 2024 15:20
@spjuhel spjuhel self-assigned this Aug 9, 2024
@spjuhel spjuhel changed the title Feature/cyclostrophic as parameter Shift cyclostrophic argument to model_kwargs in from_tracks Aug 9, 2024
spjuhel pushed a commit to CLIMADA-project/climada_petals that referenced this pull request Aug 9, 2024
@spjuhel
Copy link
Collaborator Author

spjuhel commented Aug 9, 2024

This CLIMADA-project/climada_petals@cfba26b should fix the test error for climada petal.

@chahank
Copy link
Member

chahank commented Nov 21, 2024

Has anyone reviewed this?

@spjuhel
Copy link
Collaborator Author

spjuhel commented Nov 21, 2024

Not that I know, feel free to do it :)
If I remember correctly, it is indeed ready for review!

@chahank
Copy link
Member

chahank commented Nov 21, 2024

Ok, thanks! I will see if I can do it soon. Next time please assign someone to the PR to review, or ask us to assign someone.

@NicolasColombi
Copy link
Collaborator

NicolasColombi commented Dec 13, 2024

Thank you Sam! 👍 a few remarks:

LOGGER.warning seems more appropriate to me as well, since from my understanding, the all point of keeping cyclostrophic as an argument of h_10 is to warn the user that the model is always cyclostrophic. Which brings me to the point, shall we remove cyclostrophic as an argument from h_10and add the LOGGER.warning informing on that regardless ?

I will break the line 244 in trop_cyclone.py to avoid the pylint warning: "line to long". Along the same lines, I will update the test to cover the LOGGER.warning and avoid the pylint warning of "line not covered by test".

Nice that you fixed the compatibility with petals!

Other than that, looks good to me!

@@ -100,7 +100,6 @@ def compute_angular_windspeeds(
mask_centr_close: np.ndarray,
model: int,
model_kwargs: Optional[dict] = None,
cyclostrophic: bool = False,
Copy link
Collaborator

@emanuel-schmid emanuel-schmid Dec 13, 2024

Choose a reason for hiding this comment

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

The proper way to deal with this is probably to mark the argument as deprecated rather than removing it right away. So we can keep it compatible.
If it is provided despite its deprecation insert it in model_kwargs, e.g., like this:

if cyclostrophic is not None:
    warnings.warn("yada yada", DeprecationWarning)
    model_kwargs["cyclostrophic"] = cyclostrophic

@emanuel-schmid
Copy link
Collaborator

changelog.md should mention this, because either a signature has changed or an argument got deprecated.

DeprecationWarning,
)
model_kwargs["cyclostrophic"] = cyclostrophic

Copy link
Collaborator

Choose a reason for hiding this comment

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

The proper way to deal with this is probably to mark the argument as deprecated rather than removing it right away. So we can keep it compatible. If it is provided despite its deprecation insert it in model_kwargs, e.g., like this:

if cyclostrophic is not None:
    warnings.warn("yada yada", DeprecationWarning)
    model_kwargs["cyclostrophic"] = cyclostrophic

Like this @emanuel-schmid ? see also the other edits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conventions Coding conventions or style good first issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make cyclostrophic a model parameter in the Holland windfield model interface
5 participants