Fixed galactic coord problem

[andamian]

fixes #2147

Does this look better?

[keflavich]

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

[andamian]

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]

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]

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]

@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?

[keflavich]

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]

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 Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.39%. Comparing base (97b5dd3) to head (92adbe8).
Report is 9 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]

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

[keflavich]

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

[bsipocz]

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]

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]

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]

This is good to go, thanks!