Only apply the cyclostrophic deprecation shim when the argument is passed - #1315
Open
dylanpulver wants to merge 1 commit into
Open
Only apply the cyclostrophic deprecation shim when the argument is passed#1315dylanpulver wants to merge 1 commit into
dylanpulver wants to merge 1 commit into
Conversation
compute_angular_windspeeds declares `cyclostrophic: Optional[bool] = False` but guards its deprecation shim with `if cyclostrophic is not None`. Because `False is not None`, the shim ran on every call, including the internal one from compute_windfields_sparse which never passes the argument. That raised a DeprecationWarning for an argument nobody passed, and overwrote the caller's own `model_kwargs["cyclostrophic"]` with False, making the setting unreachable through the very route the deprecation message recommends. Use None as the sentinel default, and copy model_kwargs so the shim cannot mutate the caller's dict. Existing default behaviour is unchanged: without the injected False, each model falls back to its own signature default (False for H1980, H08 and ER11, True for H10), which is what TropCyclone.from_tracks already documents. Fixes CLIMADA-project#1209
dylanpulver
requested review from
chahank,
emanuel-schmid and
peanutfun
as code owners
September 1, 2026 10:12
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes proposed in this PR:
cyclostrophicargument ofcompute_angular_windspeedsaNonesentinel default instead ofFalse, so the deprecation shim only fires when a caller actually passes it.model_kwargsbefore the shim writes into it, so a caller's dict is never modified in place.This PR fixes #1209
Diagnosis
compute_angular_windspeedsdeclarescyclostrophic: Optional[bool] = Falsebut guards the shim withif cyclostrophic is not None:. SinceFalse is not None, the shim runs on every call — including the internal one fromcompute_windfields_sparse, which never passes the argument. So aDeprecationWarningis raised for an argument nobody passed (the 44 occurrences in #1209), andmodel_kwargs["cyclostrophic"] = cyclostrophicthen overwrites the caller's own setting withFalse. That makescyclostrophicunreachable viamodel_kwargs— the exact route the deprecation message points users to, and the one documented inTropCyclone.from_tracks.Measured on
developwith the ER11 setup fromtest_er_2011_pass(r_max = 40 km, v_max = 40 m/s, f = 6.61918149e-05 1/s), at r = 30 km:_stat_er_2011(..., cyclostrophic=True)_stat_er_2011(..., cyclostrophic=False)compute_angular_windspeeds(..., model_kwargs={"cyclostrophic": True})The requested value is discarded and the caller's dict comes back as
{'cyclostrophic': False}. End to end,TropCyclone.from_tracks(tracks, centroids, model="ER11", model_kwargs={"cyclostrophic": True})returns an intensity matrix bit-identical to the default call.38.4 m/s is equation (36) of Emanuel and Rotunno (2011),
M = M_max · 2(r/r_max)² / (1 + (r/r_max)²)withM_max = r_max·v_max = 1.6e6 m²/s:1.6e6 · 2 · 0.5625 / 1.5625 / 30e3 = 38.4. The non-cyclostrophic branch adds0.5·f·r_max² = 52953.45 m²/stoM_max, giving 39.670883.Effect on results
None for existing default behaviour. Without the injected
cyclostrophic=False, each model falls back to its own signature default —Falsefor H1980, H08 and ER11,Truefor H10 — which is whatTropCyclone.from_tracksalready documents ("Default: True for H10 model, False otherwise"). H10 ignores the flag either way, so its spurious "this setting is ignored" log line also stops. No stored reference value changes; numbers move only for callers who explicitly asked forcyclostrophicand were previously ignored.Introduced in cb97195 ("Deprecate cyclostrophic instead of removing it"). Existing tests call
_stat_holland_1980and_stat_er_2011directly, so nothing covered the dispatcher's kwarg forwarding.Tests
Three tests added to
TestWindfieldHelpers, asserting literals derived from Emanuel and Rotunno (2011) eq. (36) rather than from the code:cyclostrophicviamodel_kwargsreaches the model; noDeprecationWarningunless the deprecated argument is passed;model_kwargsis not mutated.test_trop_cyclone_windfields.pygoes 9 passed → 12 passed, and all three fail against the unpatched file. Acrosstest_trop_cyclone.py+test_trop_cyclone_windfields.pythe count goes 18 passed → 21 passed with no regression, and thecyclostrophicdeprecation warnings raised by that scope drop from 43 to 0.Two checklist boxes are left unticked deliberately.
test_cross_antimeridianwasdeselected locally because it downloads the coast-distance grid from Zenodo, which
returned 504 here — CI caches that data, so it should run, but I have not proven it
green either way.
pylint --rcfile=.pylintrcreports 9.52/10 on both changed fileswith every message pre-existing and structural, and none introduced by this diff.
Written with AI assistance; the wind speeds above are function output, and 38.4 m/s
is hand-derived from the published equation.
PR Author Checklist
develop)PR Reviewer Checklist