Only apply the cyclostrophic deprecation shim when the argument is passed by dylanpulver · Pull Request #1315 · CLIMADA-project/climada_python

dylanpulver · 2026-09-01T10:12:30Z

Changes proposed in this PR:

Give the deprecated cyclostrophic argument of compute_angular_windspeeds a None sentinel default instead of False, so the deprecation shim only fires when a caller actually passes it.
Copy model_kwargs before the shim writes into it, so a caller's dict is never modified in place.

This PR fixes #1209

compute_angular_windspeeds declares cyclostrophic: Optional[bool] = False but guards the shim with if cyclostrophic is not None:. Since False is not None, the shim runs on every call — including the internal one from compute_windfields_sparse, which never passes the argument. So a DeprecationWarning is raised for an argument nobody passed (the 44 occurrences in #1209), and model_kwargs["cyclostrophic"] = cyclostrophic then overwrites the caller's own setting with False. That makes cyclostrophic unreachable via model_kwargs, the exact route the deprecation message points users to.

Measured on develop with the ER11 setup from test_er_2011_pass (r_max = 40 km, v_max = 40 m/s, f = 6.61918149e-05 1/s), at r = 30 km:

call	v_ang (m/s)
`_stat_er_2011(..., cyclostrophic=True)`	38.400000
`_stat_er_2011(..., cyclostrophic=False)`	39.670883
`compute_angular_windspeeds(..., model_kwargs={"cyclostrophic": True})`	39.670883

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)²) with M_max = r_max·v_max = 1.6e6 m²/s: 1.6e6 · 2 · 0.5625 / 1.5625 / 30e3 = 38.4. The non-cyclostrophic branch adds 0.5·f·r_max² = 52953.45 m²/s to M_max, giving 39.670883.

No change to existing default behaviour: without the injected cyclostrophic=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. 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 for cyclostrophic and were previously ignored.

Introduced in cb97195. Existing tests call _stat_holland_1980 and _stat_er_2011 directly, so nothing covered the dispatcher's kwarg forwarding. Three tests added to TestWindfieldHelpers, asserting literals derived from Emanuel and Rotunno (2011) eq. (36) rather than from the code. test_trop_cyclone_windfields.py goes 9 passed → 12 passed, and all three fail against the unpatched file. Across test_trop_cyclone.py + test_trop_cyclone_windfields.py the count goes 18 passed → 21 passed with no regression, and the cyclostrophic deprecation warnings raised by that scope drop from 43 to 0.

Two checklist boxes are left unticked deliberately. test_cross_antimeridian was
deselected 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=.pylintrc reports 9.52/10 on both changed files
with 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

PR Reviewer Checklist

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

peanutfun

@dylanpulver Thank you for your contribution! I would appreciate if you used your advertised agentic AI systems to shorten your PR description to only include relevant information. It took me some time to figure out what this PR was actually about.

While your changes are obvious and solid, the tests are not. They re-define data that is already present in other tests. Please make sure testing and reference data is only defined once, and then "shared" between tests that are using them. See my comments below.

peanutfun · 2026-09-07T09:14:02Z

+    def _er_2011_setup(self):
+        """Track/centroid setup shared by the ``compute_angular_windspeeds`` tests.
+
+        Only the second track node is asserted on, since ``compute_angular_windspeeds``
+        zeroes out the first one.
+        """
+        d_centr = KM_TO_M * np.array(
+            [[35, 70, 75, 220], [30, 150, 1000, 300]], dtype=float
+        )
+        si_track = xr.Dataset(
+            {
+                "rad": ("time", KM_TO_M * np.array([75.0, 40.0])),
+                "vmax": ("time", [35.0, 40.0]),
+                "lat": ("time", [20.0, 27.0]),
+                "cp": ("time", [4.98665369e-05, 6.61918149e-05]),
+            }
+        )
+        mask = np.array(
+            [[True, True, True, True], [True, False, True, True]], dtype=bool
+        )
+        return si_track, d_centr, mask


This is taken from the test_er_2011_pass test. Please share this information between the tests instead of re-defining it.

peanutfun · 2026-09-07T09:15:05Z

+        # guard against the setting being silently dropped, which yields the
+        # Coriolis-corrected profile instead
+        self.assertFalse(
+            np.allclose(v_ang_norm[1], self.ER11_NODE1_WITH_CORIOLIS, atol=1e-4)
+        )


This is trivial. If the data is equal to ER11_NODE1_CYCLOSTROPHIC, it cannot be equal to ER11_NODE1_WITH_CORIOLIS

Suggested change

# guard against the setting being silently dropped, which yields the

# Coriolis-corrected profile instead

self.assertFalse(

np.allclose(v_ang_norm[1], self.ER11_NODE1_WITH_CORIOLIS, atol=1e-4)

)

peanutfun · 2026-09-07T09:18:37Z

+        )
+
+    def test_compute_angular_windspeeds_no_spurious_deprecation(self):
+        """No DeprecationWarning unless the deprecated argument is actually passed."""


This only checks if there is no deprecation warning. The check for the warning when passing cyclostrophic is missing. (This is what I read the test is intended to do?)

peanutfun · 2026-09-07T09:19:10Z

+    # Reference values for the second track node (r_max = 40 km, v_max = 40 m/s,
+    # f = 6.61918149e-05 1/s) from equation (36) of Emanuel and Rotunno 2011,
+    # M = M_max * 2 * (r/r_max)^2 / (1 + (r/r_max)^2) and v = M / r.
+    # Cyclostrophic: M_max = r_max * v_max = 1.6e6 m^2/s, so at r = 30 km,
+    # v = 1.6e6 * 2 * 0.5625 / 1.5625 / 30e3 = 38.4 m/s.
+    ER11_NODE1_CYCLOSTROPHIC = [38.4, 0.0, 3.194888178913738, 10.480349344978167]
+    # Non-cyclostrophic: M_max additionally contains 0.5 * f * r_max^2 = 52953.45 m^2/s.
+    ER11_NODE1_WITH_CORIOLIS = [39.670883, 0.0, 3.300626, 10.827206]


Reference values are also already defined in the test_er_2011_pass. Please avoid re-definition and use a single data source.

dylanpulver requested review from chahank, emanuel-schmid and peanutfun as code owners September 1, 2026 10:12

dylanpulver force-pushed the fix/cyclostrophic-model-kwarg branch from aa0aeb5 to a1b74d1 Compare September 2, 2026 21:00

peanutfun requested changes Sep 7, 2026

View reviewed changes

Navigation Menu

Uh oh!

Only apply the cyclostrophic deprecation shim when the argument is passed - #1315

Only apply the cyclostrophic deprecation shim when the argument is passed#1315
dylanpulver wants to merge 1 commit into
CLIMADA-project:developfrom
dylanpulver:fix/cyclostrophic-model-kwarg

dylanpulver commented Sep 1, 2026 •

edited

Loading

Uh oh!

peanutfun left a comment

Uh oh!

peanutfun Sep 7, 2026

Uh oh!

peanutfun Sep 7, 2026

Uh oh!

peanutfun Sep 7, 2026

Uh oh!

peanutfun Sep 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

dylanpulver commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Author Checklist

PR Reviewer Checklist

Uh oh!

peanutfun left a comment

Choose a reason for hiding this comment

Uh oh!

peanutfun Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

peanutfun Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

peanutfun Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

peanutfun Sep 7, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dylanpulver commented Sep 1, 2026 •

edited

Loading