Skip to content

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

Open
dylanpulver wants to merge 1 commit into
CLIMADA-project:developfrom
dylanpulver:fix/cyclostrophic-model-kwarg
Open

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

Conversation

@dylanpulver

Copy link
Copy Markdown

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

Diagnosis

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, and the one documented in TropCyclone.from_tracks.

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 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)²) 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.

Effect on results

None for 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 ("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 for cyclostrophic and were previously ignored.

Introduced in cb97195 ("Deprecate cyclostrophic instead of removing it"). Existing tests call _stat_holland_1980 and _stat_er_2011 directly, 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: cyclostrophic via model_kwargs reaches the model; no DeprecationWarning unless the deprecated argument is passed; model_kwargs is not mutated. 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
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.

1 participant