Conversation
… into rel/v1.12.0
…#889) * Fix hard-coded radius value for parachute added mass calculation Calculate radius from cd_s using a typical hemispherical parachute drag coefficient (1.4) when radius is not explicitly provided. This fixes drift distance calculations for smaller parachutes like drogues. Formula: R = sqrt(cd_s / (Cd * π)) Closes #860 Co-authored-by: Gui-FernandesBR <63590233+Gui-FernandesBR@users.noreply.github.com> Address code review: improve docstrings and add explicit None defaults Co-authored-by: Gui-FernandesBR <63590233+Gui-FernandesBR@users.noreply.github.com> Add CHANGELOG entry for PR #889 Co-authored-by: Gui-FernandesBR <63590233+Gui-FernandesBR@users.noreply.github.com> Update rocket.add_parachute to use radius=None for consistency Changed the default radius from 1.5 to None in the add_parachute method to match the Parachute class behavior. This ensures consistent automatic radius calculation from cd_s across both APIs. Co-authored-by: Gui-FernandesBR <63590233+Gui-FernandesBR@users.noreply.github.com> Refactor Parachute class to remove hard-coded radius value and introduce drag_coefficient parameter for radius estimation Fix hard-coded radius value for parachute added mass calculation Calculate radius from cd_s using a typical hemispherical parachute drag coefficient (1.4) when radius is not explicitly provided. This fixes drift distance calculations for smaller parachutes like drogues. Formula: R = sqrt(cd_s / (Cd * π)) Closes #860 Co-authored-by: Gui-FernandesBR <63590233+Gui-FernandesBR@users.noreply.github.com> Add CHANGELOG entry for PR #889 Co-authored-by: Gui-FernandesBR <63590233+Gui-FernandesBR@users.noreply.github.com> Refactor Parachute class to remove hard-coded radius value and introduce drag_coefficient parameter for radius estimation MNT: Extract noise initialization to fix pylint too-many-statements in Parachute.__init__ Co-authored-by: Gui-FernandesBR <63590233+Gui-FernandesBR@users.noreply.github.com> * Refactor environment method access in controller test for clarity * fix pylint * fix comments * avoid breaking change with drag_coefficient * reafactors Parachute.__init__ method * fix tests --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Gui-FernandesBR <63590233+Gui-FernandesBR@users.noreply.github.com> Co-authored-by: Gui-FernandesBR <guilherme_fernandes@usp.br>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #943 +/- ##
===========================================
+ Coverage 80.27% 80.93% +0.66%
===========================================
Files 104 107 +3
Lines 12769 14047 +1278
===========================================
+ Hits 10250 11369 +1119
- Misses 2519 2678 +159 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
MateusStano
added
Bug
|
I will take a look at this one over the weekend |
| Parameters | ||
| ---------- | ||
| lat : float | ||
| Latitude in degrees. |
There was a problem hiding this comment.
| Latitude in degrees. | |
| Latitude in degrees, ranging from -90 to 90 |
|
|
||
|
|
||
| def find_longitude_index(longitude, lon_list): | ||
| def find_longitude_index(longitude, lon_list): # pylint: disable=too-many-statements |
There was a problem hiding this comment.
can you solve the issue instead?
| def find_longitude_index(longitude, lon_list): # pylint: disable=too-many-statements | |
| def find_longitude_index(longitude, lon_list): |
|
|
||
|
|
||
| def find_time_index(datetime_date, time_array): | ||
| def find_time_index(datetime_date, time_array): # pylint: disable=too-many-statements |
There was a problem hiding this comment.
same here...
| def find_time_index(datetime_date, time_array): # pylint: disable=too-many-statements | |
| def find_time_index(datetime_date, time_array): |
There was a problem hiding this comment.
Pull request overview
This PR updates RocketPy’s forecast data ingestion to use UCAR THREDDS OPeNDAP endpoints (fixing NOMADS breakage) and introduces mapping support for both THREDDS-style and legacy NOMADS-style variable naming, while explicitly disabling unavailable latest-model shortcuts (GEFS/HIRESW).
Changes:
- Switch GFS/NAM/RAP latest-model fetchers to UCAR THREDDS “Best” datasets and add projected-grid handling (Lambert Conformal) in the Environment ingestion path.
- Expand
WeatherModelMappingwith explicit*_LEGACYdictionaries and add runtime mapping resolution/fallback when dataset variables don’t match the selected mapping. - Update docs/tests/changelog to reflect THREDDS migration and the temporary unavailability of GEFS/HIRESW shortcuts.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
rocketpy/environment/fetchers.py |
Migrates GFS/NAM/RAP URLs to UCAR THREDDS; adds HRRR/AIGFS fetcher helpers. |
rocketpy/environment/tools.py |
Adds Lambert Conformal projection conversion and rewrites index-finding helpers to work directly with netCDF variables (incl. descending grids). |
rocketpy/environment/weather_model_mapping.py |
Adds THREDDS-first mappings plus explicit *_LEGACY aliases and case-insensitive lookup. |
rocketpy/environment/environment.py |
Adds mapping compatibility checks/fallback, projected-grid support for x/y datasets, and disables GEFS/HIRESW shortcuts with explicit errors. |
tests/unit/environment/test_environment.py |
Adds unit tests for dictionary resolution behavior and legacy alias lookup. |
tests/integration/environment/test_environment.py |
Updates integration tests to expect errors for disabled GEFS/HIRESW shortcuts. |
docs/user/environment/3-further/other_apis.rst |
Documents built-in/legacy mapping names and projected-grid projection usage. |
docs/user/environment/1-atm-models/standard_atmosphere.rst |
Adds an explicit Sphinx label and minor formatting tweak. |
docs/user/environment/1-atm-models/soundings.rst |
Removes RUC-specific workflow guidance and redirects users to supported alternatives. |
docs/user/environment/1-atm-models/forecast.rst |
Updates guidance around remote OPeNDAP usage and marks HIRESW shortcut as unavailable. |
docs/user/environment/1-atm-models/ensemble.rst |
Adds Sphinx label and marks GEFS shortcut as unavailable with guidance for explicit datasets. |
CHANGELOG.md |
Adds an Unreleased “Fixed” entry for the THREDDS migration. |
Comments suppressed due to low confidence (1)
rocketpy/environment/environment.py:2109
- Ensemble datasets still require a valid
dictionary["ensemble"]mapping when the file actually has an ensemble dimension: if the mapping omits it (or__resolve_dictionary_for_datasetfalls back to a non-ensemble mapping), the code later fails ininverse_dictionary[dim]when buildingparams. Instead of treating missingensembleas “single member”, detect whether the dataset dimensions include an ensemble axis and raise a clear ValueError (or infer the ensemble dimension name) when the mapping doesn’t define it.
# Get ensemble data from file
has_ensemble_dimension = True
try:
num_members = len(data.variables[dictionary["ensemble"]][:])
except KeyError:
has_ensemble_dimension = False
num_members = 1
# Get pressure level data from file
levels = get_pressure_levels_from_file(data, dictionary)
inverse_dictionary = {v: k for k, v in dictionary.items()}
param_dictionary = {
"time": time_index,
"ensemble": range(num_members),
"level": range(len(levels)),
"latitude": (lat_index - 1, lat_index),
"longitude": (lon_index - 1, lon_index),
}
# Get dimensions
try:
dimensions = data.variables[dictionary["geopotential_height"]].dimensions[:]
except KeyError:
dimensions = data.variables[dictionary["geopotential"]].dimensions[:]
# Get params
params = tuple(param_dictionary[inverse_dictionary[dim]] for dim in dimensions)
| def __resolve_dictionary_for_dataset(self, dictionary, dataset): | ||
| """Resolve a compatible mapping dictionary for the loaded dataset. | ||
|
|
||
| If the provided mapping is incompatible with the dataset variables, | ||
| this method tries built-in mappings and falls back to the first | ||
| compatible one. | ||
| """ | ||
| if self.__dictionary_matches_dataset(dictionary, dataset): | ||
| return dictionary | ||
|
|
||
| for model_name, candidate in self.__weather_model_map.all_dictionaries.items(): | ||
| if self.__dictionary_matches_dataset(candidate, dataset): | ||
| warnings.warn( | ||
| "Provided weather mapping does not match dataset variables. " | ||
| f"Falling back to built-in mapping '{model_name}'." | ||
| ) | ||
| return candidate | ||
|
|
||
| return dictionary |
There was a problem hiding this comment.
__resolve_dictionary_for_dataset silently returns the original (still-incompatible) dictionary when no built-in mapping matches the dataset. This tends to fail later with less actionable KeyErrors. Consider raising a ValueError (or at least warning with dataset variables / expected keys) when no compatible mapping is found.
| See the :class:``rocketpy.environment.weather_model_mapping`` for some | ||
| dictionary examples. |
There was a problem hiding this comment.
The docstring text “See the :class:rocketpy.environment.weather_model_mapping …” uses an invalid Sphinx role markup (:class:``...```). This will generate Sphinx warnings and won’t link correctly; use a valid :class:reference (and ideally point toWeatherModelMapping`).
| See the :class:``rocketpy.environment.weather_model_mapping`` for some | |
| dictionary examples. | |
| See the :class:`rocketpy.environment.weather_model_mapping.WeatherModelMapping` | |
| class for some dictionary examples. |
| # Some THREDDS datasets use projected x/y coordinates. | ||
| # TODO CHECK THIS I AM NOT SURE????? |
There was a problem hiding this comment.
The “TODO CHECK THIS I AM NOT SURE?????” note reads like an unresolved uncertainty. Please either remove it, replace it with a clear TODO describing the remaining work/assumptions, or add a tracked issue link so it doesn’t ship as-is.
| # Some THREDDS datasets use projected x/y coordinates. | |
| # TODO CHECK THIS I AM NOT SURE????? | |
| # Some THREDDS datasets use projected x/y coordinates. When a | |
| # "projection" variable is provided in the mapping dictionary, convert | |
| # the launch site's geodesic coordinates to the model's projected | |
| # coordinate system before locating the nearest grid cell. |
| # Save debugging data | ||
| self.lat_array = lat_list | ||
| self.lon_array = lon_list | ||
| self.lat_array = [x1, x2] | ||
| self.lon_array = [y1, y2] | ||
| self.lon_index = lon_index | ||
| self.lat_index = lat_index | ||
| self.geopotentials = geopotentials | ||
| self.wind_us = wind_us | ||
| self.wind_vs = wind_vs | ||
| self.levels = levels | ||
| self.temperatures = temperatures | ||
| self.time_array = time_array[:].tolist() | ||
| self.time_array = [ | ||
| float(time_array[0]), | ||
| float(time_array[time_array.shape[0] - 1]), | ||
| ] |
There was a problem hiding this comment.
These “debugging data” assignments no longer match the attribute documentation at the top of the class: lat_array/lon_array are documented as 2x2 vertex matrices (per level) and time_array as the array of available dates, but here they’re set to 2-element lists and just [first,last] floats. Either restore the previous structure (or a 2x2 representation consistent with the docs) or update the attribute docs to match the new stored shapes/types.
| file_url = ( | ||
| "https://thredds.ucar.edu/thredds/dodsC/grib/NCEP/GFS/Global_0p25deg/Best" | ||
| ) |
There was a problem hiding this comment.
The implementation now pulls from UCAR THREDDS (thredds.ucar.edu), but the function docstring above still refers to “NOAA's GrADS data server”. Please update the docstring (and similarly for NAM/RAP/HRRR/AIGFS) so it reflects the actual provider/service being used.
| def fetch_hrrr_file_return_dataset(max_attempts=10, base_delay=2): | ||
| """Fetches the latest HRRR (High-Resolution Rapid Refresh) dataset from | ||
| the NOAA's GrADS data server using the OpenDAP protocol. | ||
|
|
||
| Parameters |
There was a problem hiding this comment.
fetch_hrrr_file_return_dataset is added but appears to have no call sites / shortcut wiring in Environment (searching the repo only finds this definition). Consider wiring it into the shortcut map (with mapping + tests/docs) or dropping it from this PR to avoid unused API surface.
| Mapping families | ||
| ---------------- | ||
| - Base names (for example ``GFS``, ``NAM``, ``RAP``) represent the current | ||
| default mappings used by the latest-model shortcuts and THREDDS-style | ||
| datasets. | ||
| - ``*_LEGACY`` names represent older NOMADS-style variable naming | ||
| conventions (for example ``lev``, ``tmpprs``, ``ugrdprs`` and | ||
| ``vgrdprs``) and are intended for archived or previously downloaded files. | ||
|
|
||
| Notes | ||
| ----- | ||
| - Mappings can also include optional keys such as ``projection`` for | ||
| projected grids and ``ensemble`` for member dimensions. | ||
| - The :meth:`get` method is case-insensitive, so ``"gfs_legacy"`` and | ||
| ``"GFS_LEGACY"`` are equivalent. |
There was a problem hiding this comment.
The class docstring indentation under “Mapping families” / “Notes” is inconsistent with the surrounding lines, which will likely render as a literal block when Sphinx/inspect dedents the docstring. Align those section headings/bullets to the same indentation level as the rest of the docstring text.
| Mapping families | |
| ---------------- | |
| - Base names (for example ``GFS``, ``NAM``, ``RAP``) represent the current | |
| default mappings used by the latest-model shortcuts and THREDDS-style | |
| datasets. | |
| - ``*_LEGACY`` names represent older NOMADS-style variable naming | |
| conventions (for example ``lev``, ``tmpprs``, ``ugrdprs`` and | |
| ``vgrdprs``) and are intended for archived or previously downloaded files. | |
| Notes | |
| ----- | |
| - Mappings can also include optional keys such as ``projection`` for | |
| projected grids and ``ensemble`` for member dimensions. | |
| - The :meth:`get` method is case-insensitive, so ``"gfs_legacy"`` and | |
| ``"GFS_LEGACY"`` are equivalent. | |
| Mapping families | |
| ---------------- | |
| - Base names (for example ``GFS``, ``NAM``, ``RAP``) represent the current | |
| default mappings used by the latest-model shortcuts and THREDDS-style | |
| datasets. | |
| - ``*_LEGACY`` names represent older NOMADS-style variable naming | |
| conventions (for example ``lev``, ``tmpprs``, ``ugrdprs`` and | |
| ``vgrdprs``) and are intended for archived or previously downloaded files. | |
| Notes | |
| ----- | |
| - Mappings can also include optional keys such as ``projection`` for | |
| projected grids and ``ensemble`` for member dimensions. | |
| - The :meth:`get` method is case-insensitive, so ``"gfs_legacy"`` and | |
| ``"GFS_LEGACY"`` are equivalent. |
| self.lat_array = [x1, x2] | ||
| self.lon_array = [y1, y2] | ||
| self.lon_index = lon_index | ||
| self.lat_index = lat_index | ||
| self.geopotentials = geopotentials | ||
| self.wind_us = wind_us | ||
| self.wind_vs = wind_vs | ||
| self.levels = levels | ||
| self.temperatures = temperatures | ||
| self.time_array = time_array[:].tolist() | ||
| self.time_array = [ | ||
| float(time_array[0]), | ||
| float(time_array[time_array.shape[0] - 1]), | ||
| ] |
There was a problem hiding this comment.
Same issue as in process_forecast_reanalysis: lat_array/lon_array/time_array are documented as full 2x2/available-date arrays, but here they’re truncated to 2-element lists and [first,last] floats. This is a behavioral change for anyone reading these attributes and should either be reverted or reflected in the class attribute documentation.
| def fetch_aigfs_file_return_dataset(max_attempts=10, base_delay=2): | ||
| """Fetches the latest AIGFS (Artificial Intelligence GFS) dataset from | ||
| the NOAA's GrADS data server using the OpenDAP protocol. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| max_attempts : int, optional | ||
| The maximum number of attempts to fetch the dataset. Default is 10. | ||
| base_delay : int, optional | ||
| The base delay in seconds between attempts. Default is 2. | ||
|
|
||
| Returns | ||
| ------- | ||
| netCDF4.Dataset | ||
| The AIGFS dataset. | ||
|
|
||
| Raises | ||
| ------ | ||
| RuntimeError | ||
| If unable to load the latest weather data for AIGFS. | ||
| """ | ||
| file_url = ( | ||
| "https://thredds.ucar.edu/thredds/dodsC/grib/NCEP/AIGFS/Global_0p25deg/Best" | ||
| ) | ||
| attempt_count = 0 | ||
| while attempt_count < max_attempts: | ||
| try: | ||
| return netCDF4.Dataset(file_url) | ||
| except OSError: | ||
| attempt_count += 1 | ||
| time.sleep(base_delay**attempt_count) | ||
|
|
||
| raise RuntimeError( | ||
| "Unable to load latest weather data for AIGFS through " + file_url | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
fetch_aigfs_file_return_dataset is added but appears to have no call sites / shortcut wiring in Environment (searching the repo only finds this definition). Consider wiring it into the shortcut map (with mapping + tests/docs) or dropping it from this PR to avoid unused API surface.
| def fetch_aigfs_file_return_dataset(max_attempts=10, base_delay=2): | |
| """Fetches the latest AIGFS (Artificial Intelligence GFS) dataset from | |
| the NOAA's GrADS data server using the OpenDAP protocol. | |
| Parameters | |
| ---------- | |
| max_attempts : int, optional | |
| The maximum number of attempts to fetch the dataset. Default is 10. | |
| base_delay : int, optional | |
| The base delay in seconds between attempts. Default is 2. | |
| Returns | |
| ------- | |
| netCDF4.Dataset | |
| The AIGFS dataset. | |
| Raises | |
| ------ | |
| RuntimeError | |
| If unable to load the latest weather data for AIGFS. | |
| """ | |
| file_url = ( | |
| "https://thredds.ucar.edu/thredds/dodsC/grib/NCEP/AIGFS/Global_0p25deg/Best" | |
| ) | |
| attempt_count = 0 | |
| while attempt_count < max_attempts: | |
| try: | |
| return netCDF4.Dataset(file_url) | |
| except OSError: | |
| attempt_count += 1 | |
| time.sleep(base_delay**attempt_count) | |
| raise RuntimeError( | |
| "Unable to load latest weather data for AIGFS through " + file_url | |
| ) |
Summary
Solving #933
We are now using OpenDAP with UCAR THREDDS:
The following models are not available on THREDDS and were deactivated
Particularly GEFS is very important for Monte Carlo simulations and we should look for alternatives as fast as possible
Observations
The Thredds OpenDap approach took me a long time to find information about. So in between I tried out other methods which will still be important for us to understand for the future (probably for re-implementing GEFS):
THREDDS actually has an archival version of GEFS that goes up to 2020. I think we can find other alternatives, but this could be a last resort option.
Useful Links