Skip to content

Commit bda2adc

Browse files
MNT: address Meteomatics review comments
Two points raised in the PR review: - `fetch_atmospheric_data_from_meteomatics` stamped the instant with a trailing "Z" without normalizing the timezone, so an aware datetime in a non-UTC zone was sent as the wrong instant. Aware datetimes are now converted to UTC; naive ones are documented as assumed UTC. - Degenerate sampling arguments (`query_limit=0`, resolutions below 2) reached `range()`/`linspace()` and failed with an opaque low-level error after the login had already been paid for. They are now validated up front by `_validate_meteomatics_sampling`. - `process_meteomatics_atmosphere` silently coerced any non-string model to "mix", hiding a mistake such as passing a Dataset or a path as `file` and querying the wrong model. It now accepts None (default) or a string, and raises otherwise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 85b2a92 commit bda2adc

4 files changed

Lines changed: 125 additions & 13 deletions

File tree

rocketpy/environment/environment.py

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1808,10 +1808,18 @@ def process_meteomatics_atmosphere( # pylint: disable=too-many-statements
18081808
Raises
18091809
------
18101810
ValueError
1811-
If credentials are missing, if no launch date is set, or if the API
1812-
returns no usable data.
1811+
If ``model`` is not a string, if credentials are missing, if no
1812+
launch date is set, or if the API returns no usable data.
18131813
"""
1814-
model = model or "mix"
1814+
if model is None:
1815+
model = "mix"
1816+
elif not isinstance(model, str):
1817+
# Coercing silently would hide a mistake such as passing a Dataset
1818+
# or a file path as 'file', and would query the wrong model.
1819+
raise ValueError(
1820+
f"Invalid Meteomatics model {model!r}: expected the model name as "
1821+
"a string (e.g. 'mix'), or None to use the default."
1822+
)
18151823
username = username or os.environ.get("METEOMATICS_USERNAME")
18161824
password = password or os.environ.get("METEOMATICS_PASSWORD")
18171825
if not username or not password:

rocketpy/environment/fetchers.py

Lines changed: 50 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -617,6 +617,39 @@ def levels(resolution):
617617
}
618618

619619

620+
def _validate_meteomatics_sampling(
621+
min_altitude,
622+
max_altitude,
623+
wind_resolution,
624+
temperature_pressure_resolution,
625+
query_limit,
626+
):
627+
"""Validates the sampling arguments before any request is issued.
628+
629+
Catching these here keeps a degenerate input from reaching ``linspace`` or
630+
``range``, where it would surface as an opaque low-level error (or as an
631+
empty request) after the account has already been charged for the login.
632+
633+
Raises
634+
------
635+
ValueError
636+
If the altitude range, the resolutions or the query limit are invalid.
637+
"""
638+
if min_altitude < 0:
639+
raise ValueError(
640+
"min_altitude must be non-negative (heights are above ground level)."
641+
)
642+
if max_altitude <= min_altitude:
643+
raise ValueError("max_altitude must be greater than min_altitude.")
644+
if wind_resolution < 2 or temperature_pressure_resolution < 2:
645+
raise ValueError(
646+
"wind_resolution and temperature_pressure_resolution must be at least "
647+
"2: a single altitude level is not enough to define a profile."
648+
)
649+
if query_limit < 1:
650+
raise ValueError("query_limit must be at least 1.")
651+
652+
620653
def _extract_meteomatics_json(data):
621654
"""Extracts (parameter, value) pairs from a Meteomatics JSON response.
622655
@@ -682,7 +715,9 @@ def fetch_atmospheric_data_from_meteomatics(
682715
Longitude of the launch site, in degrees.
683716
date : datetime.datetime
684717
The instant to query. It is formatted according to the Meteomatics
685-
date-time specification (``%Y-%m-%dT%H:%M:%SZ``).
718+
date-time specification (``%Y-%m-%dT%H:%M:%SZ``). Timezone-aware
719+
datetimes are converted to UTC; naive ones are assumed to be UTC
720+
already.
686721
model : str, optional
687722
The Meteomatics weather model to use. Default is ``"mix"``. Your
688723
account may not have access to every model. See
@@ -717,18 +752,24 @@ def fetch_atmospheric_data_from_meteomatics(
717752
If authentication fails, the API cannot be reached, returns an error
718753
status, or returns a malformed response.
719754
ValueError
720-
If the altitude range is invalid or the response contains an
721-
unrecognized parameter.
755+
If the altitude range, the resolutions or the query limit are invalid,
756+
or if the response contains an unrecognized parameter.
722757
"""
723-
if min_altitude < 0:
724-
raise ValueError(
725-
"min_altitude must be non-negative (heights are above ground level)."
726-
)
727-
if max_altitude <= min_altitude:
728-
raise ValueError("max_altitude must be greater than min_altitude.")
758+
_validate_meteomatics_sampling(
759+
min_altitude,
760+
max_altitude,
761+
wind_resolution,
762+
temperature_pressure_resolution,
763+
query_limit,
764+
)
729765

730766
token = fetch_meteomatics_token(username, password)
731767

768+
# The instant is sent with a trailing "Z", so an aware datetime must be
769+
# converted to UTC instead of being formatted as-is. A naive datetime is
770+
# assumed to already be in UTC.
771+
if date.tzinfo is not None:
772+
date = date.astimezone(timezone.utc)
732773
date_string = date.strftime("%Y-%m-%dT%H:%M:%SZ")
733774
parameter_map = _build_meteomatics_parameters(
734775
min_altitude, max_altitude, wind_resolution, temperature_pressure_resolution

tests/unit/environment/test_environment.py

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -481,6 +481,20 @@ def test_meteomatics_atmosphere_sets_profiles(example_euroc_env, monkeypatch):
481481
assert recorder["model"] == "mix"
482482

483483

484+
def test_meteomatics_non_string_model_raises(example_euroc_env, monkeypatch):
485+
"""Reject a non-string model instead of silently querying the default.
486+
487+
Passing a Dataset or a path as ``file`` by accident must not be coerced to
488+
``"mix"``, which would quietly query (and charge for) the wrong model.
489+
"""
490+
_patch_meteomatics_fetcher(monkeypatch)
491+
492+
with pytest.raises(ValueError, match="Invalid Meteomatics model"):
493+
example_euroc_env.set_atmospheric_model(
494+
type="Meteomatics", file=123, username="user", password="pass"
495+
)
496+
497+
484498
def test_meteomatics_reads_credentials_from_environment(example_euroc_env, monkeypatch):
485499
"""Fall back to the METEOMATICS_* environment variables for credentials."""
486500
recorder = {}

tests/unit/environment/test_fetchers.py

Lines changed: 50 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
from datetime import datetime, timezone
1+
from datetime import datetime, timedelta, timezone
22

33
import pytest
44

@@ -301,3 +301,52 @@ def test_fetch_meteomatics_data_invalid_altitude_range_raises(altitudes):
301301
date=datetime(2024, 1, 1, 12, tzinfo=timezone.utc),
302302
**altitudes,
303303
)
304+
305+
306+
@pytest.mark.parametrize(
307+
("kwargs", "message"),
308+
[
309+
({"wind_resolution": 1}, "at least"),
310+
({"temperature_pressure_resolution": 0}, "at least"),
311+
({"query_limit": 0}, "query_limit must be at least 1"),
312+
],
313+
)
314+
def test_fetch_meteomatics_data_invalid_sampling_raises(kwargs, message):
315+
"""Reject degenerate resolutions and query limits with a clear message.
316+
317+
Without the up-front check these reach ``linspace``/``range`` and fail with
318+
an opaque low-level error (or an empty request) instead.
319+
"""
320+
with pytest.raises(ValueError, match=message):
321+
fetchers.fetch_atmospheric_data_from_meteomatics(
322+
username="user",
323+
password="pass",
324+
latitude=39.0,
325+
longitude=-8.0,
326+
date=datetime(2024, 1, 1, 12, tzinfo=timezone.utc),
327+
**kwargs,
328+
)
329+
330+
331+
def test_fetch_meteomatics_data_converts_date_to_utc(monkeypatch):
332+
"""A non-UTC aware datetime must be converted, not stamped with a bare Z.
333+
334+
The request path carries the instant with a trailing "Z", so 12:00 at
335+
UTC+03:00 has to be sent as 09:00Z.
336+
"""
337+
calls = []
338+
monkeypatch.setattr(fetchers.requests, "get", _make_fake_meteomatics_get(calls))
339+
340+
fetchers.fetch_atmospheric_data_from_meteomatics(
341+
username="user",
342+
password="pass",
343+
latitude=39.0,
344+
longitude=-8.0,
345+
date=datetime(2024, 1, 1, 12, tzinfo=timezone(timedelta(hours=3))),
346+
wind_resolution=2,
347+
temperature_pressure_resolution=2,
348+
)
349+
350+
data_calls = [c for c in calls if c[0] != fetchers.METEOMATICS_LOGIN_URL]
351+
assert data_calls, "expected at least one data request"
352+
assert all("2024-01-01T09:00:00Z" in url for url, _ in data_calls)

0 commit comments

Comments
 (0)