Skip to content

Fix get_nexrad_location mutating shared NEXRAD_LOCATIONS table - #1879

Open
Yuxiang-Ren-HUB wants to merge 3 commits into
ARM-DOE:mainfrom
Yuxiang-Ren-HUB:fix-nexrad-location-mutation
Open

Fix get_nexrad_location mutating shared NEXRAD_LOCATIONS table#1879
Yuxiang-Ren-HUB wants to merge 3 commits into
ARM-DOE:mainfrom
Yuxiang-Ren-HUB:fix-nexrad-location-mutation

Conversation

@Yuxiang-Ren-HUB

Copy link
Copy Markdown

Problem

get_nexrad_location() converts the station's elevation from feet to
meters by writing the result back into the dict it just read from:

loc = NEXRAD_LOCATIONS[station.upper()]
loc["elev"] = loc["elev"] * 0.3048
return loc["lat"], loc["lon"], loc["elev"]

loc is not a copy -- it's a reference into the shared, module-level
NEXRAD_LOCATIONS dict, so the conversion is applied permanently.
Calling get_nexrad_location() a second time for the same station
applies the conversion again, silently corrupting the result:

>>> get_nexrad_location("KTLX")[2]
369.7224          # 1213 * 0.3048, correct
>>> get_nexrad_location("KTLX")[2]
112.69138752      # converted twice, wrong

Any code path that calls this function more than once for the same
station -- e.g. processing multiple files/sweeps from the same radar
in one process -- silently gets a wrong altitude after the first call.

Fix

Store the converted elevation in a local variable and return that,
instead of writing back into the shared table. NEXRAD_LOCATIONS is
now never mutated by this function.

Testing

Added tests/io/test_nexrad_common.py (previously no dedicated test
file existed for this module):

  • test_get_nexrad_location_known_station: sanity-checks the returned
    lat/lon/elev for a known station.
  • test_get_nexrad_location_repeated_calls_are_stable: regression test
    for this bug -- calls the function 3 times for the same station and
    asserts the elevation doesn't change, and that the underlying table
    entry stays at its original (feet) value.

Both pass locally.

loc is a reference into the shared NEXRAD_LOCATIONS dict, not a copy.
Writing the feet->meters conversion back into loc["elev"] mutated the
table permanently, so a second call for the same station applied the
conversion again and silently corrupted the elevation.
Added unit tests for the nexrad_common module to verify location retrieval and stability across repeated calls.
@Yuxiang-Ren-HUB

Copy link
Copy Markdown
Author

Friendly ping on this one — it looks like no CI has run yet. I believe workflows need a maintainer to approve them for a first-time contributor.
Happy to rebase or adjust anything if that would help. Thanks!

@scollis

scollis commented Aug 12, 2026

Copy link
Copy Markdown
Member

Hey! Patience please! We are updating our contributors guide right now. We are also considering requiring submitters to know who they are as purely AI generated PRs will not be allowed. A real person must review code. We're not saying this is a pure AI generated PR but you do not have much of a GitHub history (as you said) and we need to ensure you are not an agent. If you could let us know who you are in real life that would speed things up.

@scollis

scollis commented Aug 13, 2026

Copy link
Copy Markdown
Member

Great! And greetings! And thanks for the AI disclosure. We'll get to this soon.

@Yuxiang-Ren-HUB

Copy link
Copy Markdown
Author

Thanks for the reply, and for taking the time to check.
I'm a graduate student in atmospheric science in China and I use Py-ART
in my own radar work, which is how I ran into this. get_nexrad_location
was writing the converted elevation back into the shared NEXRAD_LOCATIONS
dict, so a second call for the same station came back wrong (KTLX:
369.72 m, then 112.69 m). I reproduced it, wrote the fix and the tests,
and ran them locally before opening the PR.
I'm a real person, not an agent. I did use an AI assistant while writing
the patch, but the bug, the fix and the tests are ones I checked myself.

@Yuxiang-Ren-HUB

Copy link
Copy Markdown
Author

Thank you, much appreciated.

The previous commit left a single blank line between get_nexrad_location
and the following module-level comment block. Black requires two blank
lines after a top-level definition, so the Check Black job would have
failed once CI is allowed to run. No functional change.
@Yuxiang-Ren-HUB
Yuxiang-Ren-HUB force-pushed the fix-nexrad-location-mutation branch from 350b88a to 80964ea Compare August 14, 2026 00:38
Comment thread pyart/io/nexrad_common.py
# converted the already-converted value again, silently corrupting
# the elevation (e.g. KTLX: 1213 ft -> 369.72 m on the first call,
# then -> 112.7 m on the second).
elev_m = loc["elev"] * 0.3048

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the behavior table values changes should be preserved as some code in the wild may be relying on it. We may add units to know if the height is already changed in meters or not. If yes then conversion will return the values but will not change it back in the table again. This will preserved the behavior.

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.

3 participants