FIX: correct default Earth radius (10x too large) in geopotential height conversions - #1193
Closed
zjk199506 wants to merge 1 commit into
Closed
FIX: correct default Earth radius (10x too large) in geopotential height conversions#1193zjk199506 wants to merge 1 commit into
zjk199506 wants to merge 1 commit into
Conversation
The default radius of geopotential_height_to_geometric_height,
geopotential_to_height_asl and geopotential_to_height_agl was
63781370 m - 10x the WGS-84 semi-major axis - contradicting their
own docstrings ("defaulting to 6378137.0"). The docstring examples
were anchored to the 10x-radius values, masking the bug.
The main Environment processing paths already pass earth_radius
explicitly and are unaffected; the default only bites when the
helpers are reused directly (e.g. EnvironmentAnalysis surface
elevation from NOAA reanalysis 'z'), biasing heights low by
~14 m at 10 km and ~57 m at 20 km.
Defaults now use 6378137.0 m; docstring examples and a regression
test pin the correct values.
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.
Summary
The default Earth radius of the three geopotential conversion helpers in
rocketpy/tools.pyis 63781370 m — 10x the WGS-84 semi-major axis — directly contradicting their own docstrings:geopotential_to_height_aslandgeopotential_to_height_aglcarry the sameradius=63781370default, and the docstring examples of all three functions are anchored to the 10x-radius values, which masked the bug.Impact
Environmentprocessing paths already passself.earth_radiusexplicitly and are unaffected.EnvironmentAnalysiscomputes surface elevation from NOAA reanalysis surface geopotential (z) viageopotential_to_height_asl(surface_geopotential)with the default radius (environment_analysis.py:876).Changes
radius=63781370(0.0)->radius=6378137.0(WGS-84 semi-major axis, matching the value used inenvironment.pygeodesy and the existing docstrings) in all three helpersDefault is 63781370 m->Default is 6378137.0 m (WGS-84 semi-major axis)test_geopotential_height_to_geometric_height_default_radiuspins the correct conversion at 10/20 km and checks explicit-radius override still worksTesting
tests/unit/test_tools.py+ doctests ofrocketpy/tools.py: all passtests/unit/environment/test_environment.py: 145 passed (explicit-radius call sites unchanged)blackclean