Convert gauge URL lookups to O(1) dictionary lookups (#50)#59
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors gauge URL resolution in murgtools/getdata/getDataFRF.py by replacing large if/elif lookup chains with module-level dictionaries for O(1) lookups, and adds unit tests to validate the new lookup-table behavior.
Changes:
- Introduces
_WAVE_GAUGE_URLSand_WL_GAUGE_CONFIGmodule-level dictionaries to replace conditional gauge URL selection logic. - Updates
_waveGaugeURLlookup()and_wlGageURLlookup()to use dictionary.get(...)lookups. - Adds a new
TestLookupTableDictionariestest class covering importability, structure, and representative alias behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
murgtools/getdata/getDataFRF.py |
Replaces gauge URL if/elif chains with module-level dictionary lookup tables and updates the lookup methods accordingly. |
tests/test_getDataFRF.py |
Adds tests validating dictionary presence/structure and key alias behavior for wave and water-level gauge lookups. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+67
to
+73
| # xp250m pressure (note: '8' maps here, not xp200m - preserved from original) | ||
| 'xp250m': 'oceanography/waves/xp250m/xp250m.ncml', | ||
| 'xp250': 'oceanography/waves/xp250m/xp250m.ncml', | ||
| # xp200m pressure | ||
| '8': 'oceanography/waves/xp200m/xp200m.ncml', | ||
| 'xp200m': 'oceanography/waves/xp200m/xp200m.ncml', | ||
| 'xp200': 'oceanography/waves/xp200m/xp200m.ncml', |
Comment on lines
1432
to
1434
| 340m pressure can be ['xp340m', 'xp340'] | ||
|
|
||
| 250m pressure can be ['8', 'xp250m', 'xp250'] | ||
|
|
||
| 250m pressure can be ['xp250m', 'xp250'] | ||
| 200m pressure can be [8, 'xp200m', 'xp200'] |
Comment on lines
+1459
to
+1461
| obs._waveGaugeURLlookup('WaveRider-26m') | ||
| assert 'waverider-26m' in obs.dataloc | ||
|
|
SBFRF
added a commit
that referenced
this pull request
Jul 16, 2026
Fixes: - Update docstrings to remove '8' from xp200m options (8 maps to xp250m) - Docstrings now correctly show: 250m pressure can be [8, 'xp250m', 'xp250'] - Note: test_gauge_8_maps_to_xp250m already covers this behavior Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
SBFRF
added a commit
that referenced
this pull request
Jul 16, 2026
Addresses PR #59 review comment about ensuring the historically ambiguous numeric alias '8' is tested. The dictionary lookup preserves the original if-elif behavior where xp250m was matched first. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Comment on lines
+1452
to
+1454
| # Input validation | ||
| if gaugenumber is None: | ||
| raise InvalidGaugeError(gaugenumber, message='Gauge number cannot be None.') |
Comment on lines
+1493
to
+1495
| # Input validation | ||
| if gaugenumber is None: | ||
| raise InvalidGaugeError(gaugenumber, message='Gauge number cannot be None.') |
Comment on lines
+1422
to
+1423
| assert len(_WAVE_GAUGE_URLS) > 50 # Should have many entries | ||
| assert len(_WL_GAUGE_CONFIG) > 20 # Should have many entries |
Comment on lines
+1565
to
+1569
| def test_gauge_8_maps_to_xp250m(self): | ||
| """Test that gauge '8' maps to xp250m (first match in original if-elif chain).""" | ||
| from murgtools.getdata.getDataFRF import getObs, _WAVE_GAUGE_URLS | ||
| import datetime as DT | ||
|
|
Comment on lines
+1452
to
+1454
| # Input validation | ||
| if gaugenumber is None: | ||
| raise InvalidGaugeError(gaugenumber, message='Gauge number cannot be None.') |
Comment on lines
+1493
to
+1495
| # Input validation | ||
| if gaugenumber is None: | ||
| raise InvalidGaugeError(gaugenumber, message='Gauge number cannot be None.') |
Implements Issue #50 Task 2.2 optimization. Replaces ~150 lines of if-elif statements in _waveGaugeURLlookup() and _wlGageURLlookup() with module-level dictionaries for O(1) lookup performance. Adds 10 new tests for dictionary lookup behavior, edge cases, and all gauge aliases. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Refinements to dictionary lookup implementation: - Fix '8' key to map to xp250m (matches original if-elif first-match behavior) - Add explicit None input validation with clear error messages - Update docstrings to document Raises behavior - Add 5 new edge case tests: gauge '8' mapping, None handling, empty string, float inputs, and case insensitivity Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Fixes: - Update docstrings to remove '8' from xp200m options (8 maps to xp250m) - Docstrings now correctly show: 250m pressure can be [8, 'xp250m', 'xp250'] - Note: test_gauge_8_maps_to_xp250m already covers this behavior Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Addresses PR #59 review comment about ensuring the historically ambiguous numeric alias '8' is tested. The dictionary lookup preserves the original if-elif behavior where xp250m was matched first. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
SBFRF
force-pushed
the
feature/issue-50-lookup-table-optimization
branch
from
July 16, 2026 19:24
d23c03d to
a758af0
Compare
Comment on lines
1576
to
1580
| 4.5m AWAC can be [5, 'awac-4.5m', 'Awac-4.5m'] | ||
| 3.5m aquadopp can be [6, 'adop-3.5m', 'aquadopp 3.5m'] | ||
| 200m pressure can be [8, 'xp200m', 'xp200'] | ||
| 200m pressure can be ['xp200m', 'xp200'] | ||
| 150m pressure can be [9, 'xp150m', 'xp150'] | ||
| 125m pressure can be [10, 'xp125m', 'xp125'] |
Comment on lines
3258
to
3261
| 4.5m AWAC can be [5, 'awac-4.5m', 'Awac-4.5m'] | ||
| 3.5m aquadopp can be [6, 'adop-3.5m', 'aquadopp 3.5m'] | ||
| 200m pressure can be [8, 'xp200m', 'xp200'] | ||
| 200m pressure can be ['xp200m', 'xp200'] | ||
| 150m pressure can be [9, 'xp150m', 'xp150'] |
Comment on lines
+1566
to
+1570
| from murgtools.getdata.getDataFRF import _WAVE_GAUGE_URLS, _WL_GAUGE_CONFIG | ||
| assert isinstance(_WAVE_GAUGE_URLS, dict) | ||
| assert isinstance(_WL_GAUGE_CONFIG, dict) | ||
| assert len(_WAVE_GAUGE_URLS) > 50 # Should have many entries | ||
| assert len(_WL_GAUGE_CONFIG) > 20 # Should have many entries |
- Fix InvalidGaugeError messages for None input to include 'None' value - Remove duplicate test (test_wave_gauge_lookup_8_maps_to_xp250m) - Replace brittle dictionary size assertions with required key checks Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Addresses PR #59 review comments: - _wlGageURLlookup: Add 8 to 200m pressure options (matches _WL_GAUGE_CONFIG) - getWaveSpecModel: Add 8 to 200m pressure options (matches implementation) Note: In _WL_GAUGE_CONFIG, integer 8 maps to xp200m (water level). This is distinct from _WAVE_GAUGE_URLS where string '8' maps to xp250m (waves). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
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
_waveGaugeURLlookup()(~90 if-elif conditions) and_wlGageURLlookup()(~12 conditions) to O(1) dictionary lookupsTest plan
InvalidGaugeError🤖 Generated with Claude Code