Conversation
|
|
||
| .. Matplotlib | ||
| .. _Matplotlib: https://matplotlib.org/ | ||
| .. |Patch| replace:: `~matplotlib.patches.Patch` |
There was a problem hiding this comment.
I don't see any region to accept any of these changes.
docs/api.rst
Outdated
| .. automodapi:: regions.io.ds9 | ||
| :no-inheritance-diagram: | ||
|
|
||
| .. automodapi:: regions.io.stcs | ||
| :no-inheritance-diagram: |
There was a problem hiding this comment.
Probably should reject this change - no reason to exclude inheritance diagrams, right?
regions/io/stcs/README.md
Outdated
| ## Contributing | ||
|
|
||
| When contributing to the STC-S module: | ||
|
|
||
| 1. Follow the existing code style and patterns from the DS9 module | ||
| 2. Add comprehensive tests for new functionality | ||
| 3. Update documentation and examples | ||
| 4. Ensure round-trip conversions work correctly | ||
| 5. Test with various coordinate systems and shapes | ||
|
|
There was a problem hiding this comment.
no need for this - these were basically instructions to the AI
regions/io/stcs/README.md
Outdated
|
|
||
| ## Known Limitations | ||
|
|
||
| - Complex STC-S features like time coordinates are not yet supported |
There was a problem hiding this comment.
... weird commentary, are they so complex?
regions/io/stcs/README.md
Outdated
| ## Known Limitations | ||
|
|
||
| - Complex STC-S features like time coordinates are not yet supported | ||
| - Union, intersection, and other compound operations are not implemented |
There was a problem hiding this comment.
seems reasonable - does the standard even support them?
There was a problem hiding this comment.
yes: "Optionally, compound sub-phrases may be recognized for compound Regions. Under those rules the CoordinateArea operational identifier (Union, Intersection, or Difference) precedes the CoordinateFrame component which, in turn, is followed by two or more (if applicable) CoordinateArea components; these CoordinateArea identifiers may be preceded by the negation operator Not. The arguments for these operators must be enclosed in parentheses. For Union and Intersection the argument may contain two or more elements; for Difference it must contain exactly two; and for Not it must contain only one. The enclosure in parentheses is merely intended to allow nesting and avoid ambiguities."
| result : bool | ||
| Returns `True` if the given file is an STC-S region file. | ||
| """ | ||
| all_exten = ('.stcs', '.stc', '.stcs.txt', '.stc.txt') |
There was a problem hiding this comment.
Do any of these files exist in the wild? I'm happy enough to establish this as a new filename convention otherwise.
regions/io/stcs/connect.py
Outdated
| stcs_keywords = ['Circle', 'Ellipse', 'Box', 'Polygon', 'Position', | ||
| 'ICRS', 'FK5', 'FK4', 'GALACTIC', 'ECLIPTIC'] |
There was a problem hiding this comment.
these are not unique - not good to use.
| } | ||
|
|
||
| # Regular expressions for parsing STC-S strings | ||
| STCS_PATTERNS = { |
There was a problem hiding this comment.
these are hard to review; I'm just trusting for now
regions/io/stcs/example_usage.py
Outdated
| @@ -0,0 +1,130 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
this whole file is not needed; it's basically a documentation example. So I'll cut it.
|
I did a pretty simple check on a couple dozen local files, and it seems to have worked nicely. I'd like to come up with some real-world use cases, e.g., https://cds-astro.github.io/aladin-lite/A.html#.footprintsFromSTCS, before we consider merging this, but it's at least ready for some discussion. |
|
Hi @keflavich -- just dropping a note regarding real-world use cases: MAST uses STC-S s_regions in lots of places (internal metadata processing and serialization to s_region strings; Jdaviz has a custom serialization for s_region string <-> region in both directions). (As a next step beyond this, STC-S serialization would also be helpful with spherical regions, for some MAST metadata calculation purposes.) |
2 participants
This initial implementation is 100% AI-generated, so do not merge it - I am pushing it so that I can perform code review and to trigger further discussion of #21. Also, I wanted to get some STC regions for use with aladin-lite, and I don't have any other way to make them.