BUG: vector SkyCoords are not supported

[bsipocz]

I was running into the issue of SIAv2 is not working with scalar coordinates (#305), so I made them a vector SkyCoord, which is working even less, e.g. _validate_pos() in dal.params wrongly assumes ra and dec from it. IMO the whole position validation should be rewritten and make more reliant on astropy.coordinates as most of the logic is already been done there.

>>> seip.search(pos=SkyCoord([151.1, 151.1], [2.0, 2.0], unit="deg"), size=0.0)
File ~/munka/devel/pyvo/pyvo/dal/sia2.py:196, in SIAService.search(self, pos, band, time, pol, field_of_view, spatial_resolution, spectral_resolving_power, exptime, timeres, publisher_did, facility, collection, instrument, data_type, calib_level, target_name, res_format, maxrec, session, **kwargs)
    181 def search(self, pos=None, band=None, time=None, pol=None,
    182            field_of_view=None, spatial_resolution=None,
    183            spectral_resolving_power=None, exptime=None,
   (...)
    186            target_name=None, res_format=None, maxrec=None, session=None,
    187            **kwargs):
    188     """
    189     Performs a SIAv2 search against a SIAv2 service
    190 
   (...)
    194 
    195     """
--> 196     return SIAQuery(self.query_ep, pos=pos, band=band,
    197                     time=time, pol=pol,
    198                     field_of_view=field_of_view,
    199                     spatial_resolution=spatial_resolution,
    200                     spectral_resolving_power=spectral_resolving_power,
    201                     exptime=exptime, timeres=timeres,
    202                     publisher_did=publisher_did,
    203                     facility=facility, collection=collection,
    204                     instrument=instrument, data_type=data_type,
    205                     calib_level=calib_level, target_name=target_name,
    206                     res_format=res_format, maxrec=maxrec,
    207                     session=session, **kwargs).execute()

File ~/munka/devel/pyvo/pyvo/dal/sia2.py:269, in SIAQuery.__init__(self, url, pos, band, time, pol, field_of_view, spatial_resolution, spectral_resolving_power, exptime, timeres, publisher_did, facility, collection, instrument, data_type, calib_level, target_name, res_format, maxrec, session, **kwargs)
    266 super().__init__(url, session=session)
    268 for pp in _tolist(pos):
--> 269     self.pos.add(pp)
    271 for bb in _tolist(band):
    272     self.band.add(bb)

File ~/munka/devel/pyvo/pyvo/dal/params.py:258, in AbstractDalQueryParam.add(self, item)
    257 def add(self, item):
--> 258     if item in self:
    259         return
    260     self._data.append(item)

File ~/munka/devel/pyvo/pyvo/dal/params.py:279, in AbstractDalQueryParam.__contains__(self, item)
    277 def __contains__(self, item):
    278     # check dal format for duplications since the quantities are known
--> 279     return self.get_dal_format(item) in self.dal

File ~/munka/devel/pyvo/pyvo/dal/params.py:303, in PosQueryParam.get_dal_format(self, val)
    298 def get_dal_format(self, val):
    299     """
    300     formats the tuple values into a string to be sent to the service
    301     entries in values are either quantities or assumed to be degrees
    302     """
--> 303     self._validate_pos(val)
    304     if len(val) == 3:
    305         shape = 'CIRCLE'

File ~/munka/devel/pyvo/pyvo/dal/params.py:355, in PosQueryParam._validate_pos(self, pos)
    353     self._validate_dec(m)
    354 else:
--> 355     self._validate_ra(m)

File ~/munka/devel/pyvo/pyvo/dal/params.py:359, in PosQueryParam._validate_ra(self, ra)
    357 def _validate_ra(self, ra):
    358     if not isinstance(ra, Quantity):
--> 359         ra = ra * u.deg
    360     if ra.to(u.deg).value < 0 or ra.to(u.deg).value > 360.0:
    361         raise ValueError('Invalid ra: {}'.format(ra))

TypeError: unsupported operand type(s) for *: 'SkyCoord' and 'Unit'

[msdemlei]

On Wed, Jan 25, 2023 at 01:36:59PM -0800, Brigitta Sipőcz wrote: it. IMO the whole position validation should be rewritten and make more reliant on astropy.coordinates as most of the logic is already been done there.

That sounds like an excellent plan to me.

[bsipocz]

Anyway, here is a list of POS inputs for single regions that I think we can easily support with what we have if we table the idea of queries for multiple objects. We already test for most of these, and the others are an easy fix (partial copy from test_sia2.py):

    circle_pos (coordinate pair + radius) = [(2, 4, 0.0166),
                  (2, 4, 0.0028972 * u.rad),
                  (SkyCoord(2, 4, unit='deg'), 0.166 * u.deg)]
    range_pos (two coordinate pairs) = [(12, 12.5, 34, 36),
                 (0.209 * u.rad, 0.218 * u.rad, 0.593 * u.rad, 0.628 * u.rad),
                 (SkyCoord(12, 34, unit='deg'), SkyCoord(12.5, 36, unit='deg'))]
    polygon_pos (any 3+ coordinate pairs) = [(12.0 * u.deg, 34.0 * u.deg,
                    14.0 * u.deg, 35.0 * u.deg,
                    14.0 * u.deg, 36.0 * u.deg,
                    12.0 * u.deg, 35.0 * u.deg),
                   (SkyCoord(12.0 * u.deg, 34.0 * u.deg),
                    SkyCoord(14.0 * u.deg, 35.0 * u.deg),
                    SkyCoord(14.0 * u.deg, 36.0 * u.deg),
                    SkyCoord(12.0 * u.deg, 35.0 * u.deg))]

Now, the question of the vector SkyCoords are interesting, how should we distinguish between a single-position polygon search and multiple circle searches?

[bsipocz]

cc @keflavich

[bsipocz]

OK, so vector SkyCoord, maybe we only support the CIRCLE case?

[msdemlei]

On Mon, Sep 16, 2024 at 05:37:53PM -0700, Brigitta Sipőcz wrote: Originally I was thinking here to support a vector skycoord to specify multiple position queries all at once, but I don't think that would work with the servers, so we may just leave the looping to the user?

Correctly implemented services should evaluate as many appearences of POS as there are. I frankly fought against these multi-valued parameters, but not that they're there we should use them. If this then breaks left and right, we (and the service operators) will have learned something, too :-) So... I'm all for turning multiple geometries into a sequence of POS parameters.

…

-- Markus

[bsipocz]

but not that they're there we should use them. If this
then breaks left and right

So far the only limitation I run into was blindly following and incrementally extending the current pyvo logic at parsing, and nothing on the actual query side. I even deleted those side-tracking comments I made where I wondered about what services will support, without remembering that you'll still see them in your emails :)