Repository navigation
fix(acquisition): read OpenSky state rows through the column-name table - #154
Conversation
`_STATE_COLUMNS_NEW` was added unused in #92 while `state_row_to_record` hard-coded eleven magic indexes; the comment claiming the function used it named a function that never existed. Rename it to `_STATE_COLUMNS`, derive a name -> index map from it, and index rows by documented column name so the /api/states/all column order is written once. No behaviour change: every name resolves to the index that was hard-coded, checked against the OpenSky REST docs column table and by a differential test of the old and new function over 20,075 rows (0 mismatches). Add a test that tells `time_position` from `last_contact`: the shared fixture gives both the same value, so swapping them went unnoticed by every test. Resolves GitHub Code Quality finding #24 (py/unused-global-variable). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Summary
Resolves GitHub Code Quality finding #24 (
py/unused-global-variable): "The global variable_STATE_COLUMNS_NEWis not used" inpython/acquisition/opensky.py.The tuple of OpenSky
/api/states/allcolumn names was added in #92 but never read:state_row_to_recordhard-coded eleven magic indexes instead, and the comment above the tuple said it was used by_state_row_to_record, a function that never existed. Rather than delete the table, this PR uses it:_STATE_COLUMNS_NEW->_STATE_COLUMNS(the suffix referred to nothing; private, zero references anywhere in the repo)_COL, a name -> index map, from the tuplerow[_COL["geo_altitude"]]instead ofrow[13])The column order is now written once. A mistyped name fails loudly with
KeyError, where a mistyped number silently read the wrong column.No behaviour change
IndexErroras before (kept on purpose; see follow-ups).One new test
Mutation testing of the tuple showed that swapping
time_positionandlast_contactpassed every test, because the shared fixture gives both the same value (a gap that predates this change).test_timestamp_is_time_position_not_last_contactcloses it; with the two names swapped it fails, as it should. It uses only the public function.Checks (the
python-training-treelane's commands, run inpython/)ruff check .: passedpyright(strict): 0 errors, 0 warningspytest -q: 119 passed, 3 skipped (torch / onnxruntime extras not installed, by design)OpenSpec Validateis expected to be red: it fails ondevelopalready (26 main specs with the placeholder Purpose that OpenSpec 1.13 rejects), which a separate PR addresses.Found while verifying, NOT in this PR
The numeric -> letter emitter-category conversion in the same function is shifted by one relative to the OpenSky docs, where 0 = no information, 1 = "No ADS-B Emitter Category Information", 2 = Light ... 6 = Heavy, 7 = High Performance, 8 = Rotorcraft, 9 = Glider ... 14 = UAV. The code maps 1..7 to A1..A7, so:
A1A3A6B1B7It shows in the checked-in sample: the only
A1aircraft intest-data/trajectories/opensky-sample.parquetare DLH85N and EJU64AM, both airliners. A live query on 2026-09-19 showed the same (a British Airways long-haul with category 6 = Heavy lands inother). Fixing it changes training labels, a test expectation (4should giveA3), the checked-in sample and its recorded SHA-256, and any OpenSky data already captured, so it needs its own branch and a decision on re-labelling.Smaller observations, also left alone: a malformed short row escapes
fetch_state_vectorsas a bareIndexErrorrather than anOpenSkyError;NUM_CLASSESand the Decision 8 category table are each defined twice (they agree today).🤖 Generated with Claude Code