Skip to content

Fixed galactic coord problem#3105

Merged
bsipocz merged 1 commit into
astropy:mainfrom
andamian:alma_coords
Oct 11, 2024
Merged

Fixed galactic coord problem#3105
bsipocz merged 1 commit into
astropy:mainfrom
andamian:alma_coords

Conversation

@andamian

@andamian andamian commented Sep 25, 2024

Copy link
Copy Markdown

fixes #2147

Does this look better?
distribution

@andamian andamian marked this pull request as draft September 25, 2024 22:15
@keflavich

Copy link
Copy Markdown
Contributor

Could you repost the original query here? I think this looks better.

@andamian

andamian commented Sep 25, 2024

Copy link
Copy Markdown
Author

before

select * from ivoa.obscore WHERE (0.0<=spatial_resolution AND spatial_resolution<=0.5) AND (INTERSECTS(RANGE_S2D(268.37542277754466,264.47377034405804,-29.96387952993674,-27.88035232035121), s_region) = 1) AND science_observation='T' AND data_rights='Public'

now

select * from ivoa.obscore WHERE (0.0<=spatial_resolution AND spatial_resolution<=0.5) AND (INTERSECTS(RANGE_S2D(264.47377034405804,268.37542277754466,-29.96387952993674,-27.88035232035121), s_region) = 1) AND science_observation='T' AND data_rights='Public'

I think that because of the order it was intersecting the wrong range.

Note that this is slightly larger than the original galactic range because in ICRS the range is "tilted" I would need to use a polygon to be more accurate. Is that acceptable?

@andamian

Copy link
Copy Markdown
Author

The unit test fails now because SkyCoord automatically makes ra=360 into 0 and that messes up the min/max in formula. Is there a way to prevent that?

@keflavich

Copy link
Copy Markdown
Contributor

SkyCoord has a wrap_at attribute; set that to 180deg - e.g. coord.wrap_at(180*u.deg).

Yes, this is acceptable, but we should note the limitations, and particularly that searches aligned with the Galactic plane are not supported. I think that's the right interpretation of what you just said

@andamian

Copy link
Copy Markdown
Author

@keflavich - the crude translation between galactic range to ICRS range is a crude approximation that probably includes extra areas. The accuracy depends on the size of the range and position. It is however better than the bug that we have right now.

A better alternative would be to approximate the galactic range with a polygon and use that in the ICRS intersect. To do that, we'd need to add more points to the polygon along the latitude parallels to keep the polygon vertices follow the parallels (and not on the spherical arc). More points means more accurate but also more complexity in executing the query. Is this worth from a practical point of view? Are queries like the one in the ticket common enough to justify the effort or the current approximation is sufficient?

@bsipocz bsipocz added this to the v0.4.8 milestone Sep 26, 2024
@keflavich

Copy link
Copy Markdown
Contributor

Let's use this workaround for now. I think the polygon approach is much better, but I'm not convinced demand is high enough right now to justify the extra work.

@andamian

andamian commented Oct 3, 2024

Copy link
Copy Markdown
Author

Just an update here. I've tested the polygon approach with all transformation on the client side and it seems to work for the most part. An option to do the geometry transformation between different reference systems on the service side with a query function has been just endorsed by IVOA and we might follow that lead for this issue.

@codecov

codecov Bot commented Oct 8, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.39%. Comparing base (97b5dd3) to head (92adbe8).
Report is 130 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3105   +/-   ##
=======================================
  Coverage   67.38%   67.39%           
=======================================
  Files         233      233           
  Lines       18417    18421    +4     
=======================================
+ Hits        12411    12415    +4     
  Misses       6006     6006           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andamian

andamian commented Oct 9, 2024

Copy link
Copy Markdown
Author

Fixed

I think this will do it. @keflavich - please review.

@andamian andamian marked this pull request as ready for review October 9, 2024 18:34

@keflavich keflavich left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm, but please verify that the query syntax (lower case and) is correct.

Comment thread astroquery/alma/tapsql.py Outdated

@bsipocz bsipocz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This was user reported, so I'm afraid will need a changelog entry. If we merge without one, just leave the label off and I'll deal with it at release time.

@andamian

andamian commented Oct 9, 2024

Copy link
Copy Markdown
Author

lgtm, but please verify that the query syntax (lower case and) is correct.

It's case insensitive but I've changed it for consistency. Also, switched to using coord.Angle to avoid the wrap up at 360. It is also more appropriate for its purpose (degree representation).

@andamian

andamian commented Oct 9, 2024

Copy link
Copy Markdown
Author

This was user reported, so I'm afraid will need a changelog entry. If we merge without one, just leave the label off and I'll deal with it at release time.

My bad. I wrongly assumed that it's not required because there are no API changes.

@bsipocz bsipocz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is good to go, thanks!

@bsipocz bsipocz merged commit 22db7f2 into astropy:main Oct 11, 2024
@andamian andamian deleted the alma_coords branch October 18, 2024 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Issue with spatial constraints in Alma.query

3 participants