Skip to content

Comments

swev-id: astropy__astropy-14369 — CDS units: correct chained division associativity; fix ascii.cds MRT unit parsing (fixes #73)#102

Open
rowan-stein wants to merge 4 commits intoastropy__astropy-14369from
update/cds-ply-regenerated
Open

swev-id: astropy__astropy-14369 — CDS units: correct chained division associativity; fix ascii.cds MRT unit parsing (fixes #73)#102
rowan-stein wants to merge 4 commits intoastropy__astropy-14369from
update/cds-ply-regenerated

Conversation

@rowan-stein
Copy link
Collaborator

This PR fixes incorrect parsing of CDS unit strings with chained division, which affected MRT files read via ascii.cds.

Summary

  • Make chained division left-associative in CDS grammar so that A/B/C/D parses as A * B^-1 * C^-1 * D^-1 (consistent with Generic and OGIP formats).
  • Continue to support leading inverse (e.g., /pixel/s(pixel*s)^-1).
  • Keep factor/exponent semantics unchanged (10+3J, kpc2).

Tests

  • Units (CDS):
    • 10+3J/m/s/kpc21000 J / (kpc2 m s)
    • 10-7J/s/kpc21e-07 J / (kpc2 s)
  • ascii.cds reader:
    • MRT-style header using the above units; resulting table columns have the correct units.

Implementation notes

  • Updated astropy/units/format/cds.py grammar; regenerated PLY tables (cds_lextab.py, cds_parsetab.py).
  • Added minimal alias mappings for CDS names (pix/pixel, vox/voxel, photon) to ensure spelled-out forms parse when used with leading division.
  • No lexer changes required.
  • Parentheses continue to override associativity; existing factor/exponent rules unchanged.

Fixes #73.

Reference: Task swev-id: astropy__astropy-14369.

@rowan-stein rowan-stein requested a review from a team December 18, 2025 16:05
Copy link

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

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

Thanks for regenerating the CDS parser assets. I verified the grammar changes, the regenerated parsetab matches those productions, the lexer table still exposes the punctuation tokens, and the new CDS/MRT tests cover the left-associative and leading-slash scenarios. The alias additions are minimal and targeted. Approved.

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