Shift cyclostrophic argument to model_kwargs in from_tracks #936
Conversation
|
This CLIMADA-project/climada_petals@cfba26b should fix the test error for climada petal. |
|
Has anyone reviewed this? |
|
Not that I know, feel free to do it :) |
|
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. |
|
Thank you Sam! 👍 a few remarks:
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 Nice that you fixed the compatibility with petals! Other than that, looks good to me! |
| mask_centr_close: np.ndarray, | ||
| model: int, | ||
| model_kwargs: Optional[dict] = None, | ||
| cyclostrophic: bool = False, |
There was a problem hiding this comment.
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|
changelog.md should mention this, because either a signature has changed or an argument got deprecated. |
| DeprecationWarning, | ||
| ) | ||
| model_kwargs["cyclostrophic"] = cyclostrophic | ||
|
|
There was a problem hiding this comment.
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
|
Can we merge ? @emanuel-schmid @spjuhel |
Changes proposed in this PR:
from_tracksfrom_trackscompute_angular_windspeedsand_compute_windfieldssuch that it uses the value in model_kwarg.LOGGER.debugtoLOGGER.warningin_compute_angular_windspeeds_h10to inform user thatcyclostrophic=Falseis ignored in this caseThis 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
develop)PR Reviewer Checklist