Support OMM NORAD catalog numbers beyond Alpha-5 range (closes #169) - #170
Open
karlhillx wants to merge 4 commits into
Open
Support OMM NORAD catalog numbers beyond Alpha-5 range (closes #169)#170karlhillx wants to merge 4 commits into
karlhillx wants to merge 4 commits into
Conversation
…lpha-5 range The Alpha-5 encoding used by TLEs caps NORAD_CAT_ID at 339999 (whose Alpha-5 form is 'Z9999'). OMM (CCSDS 502.0) is a modern XML/CSV/JSON format that carries the catalog ID as a plain integer and can therefore hold larger values. Previously, omm.initialize() called sgp4init() with the integer directly, which routed through to_alpha5() and raised ValueError for any satnum > 339999. Pass the NORAD_CAT_ID as a string from the OMM path so that sgp4init() skips the alpha-5 conversion. The full integer round-trips correctly through from_alpha5() because any digit-led string is decoded as int(s). The TLE code path is unchanged: twoline2rv() extracts a 5-char satnum_str directly from the TLE columns, so it never went through to_alpha5() in the first place. Adds test_omm_supports_satnum_beyond_alpha5_range for the reported case (NORAD_CAT_ID=999999) and test_omm_satnum_at_alpha5_boundary for the 339999/340000 boundary values.
There was a problem hiding this comment.
🟡 Changes recommended
The current unconditional str(satnum) approach alters satnum_str formatting even for in-range IDs (padding/Alpha‑5), which can break export_tle() and diverge from established satnum_str expectations.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the OMM initialization path so that NORAD catalog numbers that exceed the TLE Alpha‑5 maximum (339999) can be accepted without raising, aligning OMM handling with the fact that OMM stores catalog IDs as plain integers.
Changes:
- Adjust
sgp4/omm.pyto bypass Alpha‑5 encoding duringsgp4init()so largeNORAD_CAT_IDvalues don’t raise. - Add unit tests covering OMM initialization with NORAD IDs beyond the Alpha‑5 range and around the 339999/340000 boundary.
File summaries
| File | Description |
|---|---|
| sgp4/omm.py | Changes how NORAD_CAT_ID is passed into sgp4init() to avoid Alpha‑5 overflow for large catalog numbers. |
| sgp4/tests.py | Adds new tests ensuring OMM initialization works for large catalog numbers and boundary values. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
50
to
57
| # OMM (CCSDS 502.0) is a modern XML/CSV/JSON format that carries the | ||
| # NORAD catalog ID as a plain integer, not the 5-character TLE column. | ||
| # Pass the number as a string so that sgp4init() skips `to_alpha5()`, | ||
| # which can only encode satnums up to 339999 ('Z9999'). The full | ||
| # integer round-trips correctly through `from_alpha5()` because a | ||
| # digit-led string is decoded as a plain `int(s)`. | ||
| sat.sgp4init(gravconst, 'i', str(satnum), epoch, bstar, ndot, nddot, ecco, | ||
| argpo, inclo, mo, no_kozai, nodeo) |
Comment on lines
+877
to
+886
| def test_omm_satnum_at_alpha5_boundary(): | ||
| # 339999 is the last value that fits in Alpha-5 ('Z9999'). It must | ||
| # still work through the OMM path, both with the legacy integer-form | ||
| # representation that to_alpha5() would produce and as a raw int. | ||
| for norad_id in (339999, 340000): | ||
| csv = MARIO_CSV.replace('55123', str(norad_id)) | ||
| fields = next(omm.parse_csv(StringIO(csv))) | ||
| sat = Satrec() | ||
| omm.initialize(sat, fields) | ||
| assertEqual(sat.satnum, norad_id) |
…bility Co-authored-by: karlhillx <13650232+karlhillx@users.noreply.github.com>
Co-authored-by: karlhillx <13650232+karlhillx@users.noreply.github.com>
Co-authored-by: karlhillx <13650232+karlhillx@users.noreply.github.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.
Closes #169
Problem
omm.initialize()raisedValueError: satellite number cannot exceed 339999for any NORAD_CAT_ID greater than 339999. This is a problem because OMM (CCSDS 502.0) is a modern XML/CSV/JSON format that carries the catalog ID as a plain integer — not the 5-character TLE column where the Alpha-5 encoding is needed to fit 6-digit numbers into 5 columns.Real-world examples that hit this:
The bug reproduced with
NORAD_CAT_ID=999999againstmaster.Root cause
omm.initialize()passedNORAD_CAT_ID(an int) directly tosgp4init(), which callsto_alpha5()whenever itssatnargument is an int.to_alpha5()can only encode satnums up to 339999 (whose Alpha-5 form is'Z9999') and raises for anything larger.The TLE path never had this problem —
twoline2rv()extractssatrec.satnum_strdirectly from the 5-character column in the TLE and passes it as a string tosgp4init(), bypassingto_alpha5()entirely.Fix
In
sgp4/omm.py, convert the integer NORAD_CAT_ID to a string before passing it tosgp4init(). The string takes theif isinstance(satn, int):branch into a no-op, soto_alpha5()is skipped. The full integer round-trips correctly becausefrom_alpha5()decodes any digit-led string asint(s).The TLE code path is unchanged.
Behavior after the fix
Verified for satnums in the range [5, 9999999]:
'5''55123''339999''340000''999999''9999999'Note: in the OMM path,
satnum_stris the raw digit string (no zero-padding), which is fine for round-trip and propagation but meansexport_tle()cannot represent such a satrec back as a TLE. That's an inherent limitation of the TLE format itself (the column is exactly 5 characters), not a regression.Tests
Added two tests in
sgp4/tests.py:test_omm_supports_satnum_beyond_alpha5_range— exercises the reported case (NORAD_CAT_ID=999999) and asserts bothsat.satnumandsat.satnum_strround-trip correctly.test_omm_satnum_at_alpha5_boundary— checks both 339999 (Alpha-5 max, the legacy encoding boundary) and 340000 (one past, the previously broken case) via the OMM path.test_satnum_that_is_too_largestill raisesValueErrorfor directsat.sgp4init(int)calls with int > 339999, preserving the existing API contract for callers that go through the legacy int→Alpha-5 path.Test results
47 tests pass (45 pre-existing + 2 new). Pre-existing pyflakes warning on
sgp4/conveniences.py:9is unchanged.Diff stat