Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature skycoord #591

Merged
merged 15 commits into from
Oct 1, 2024
Binary file modified docs/mivot/_images/mangoEpochPosition.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
21 changes: 21 additions & 0 deletions docs/mivot/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,27 @@ with the `astropy.io.votable` API:

In this case, it is up to the user to ensure that the read data rows are those mapped by the Mivot annotations.

Get a SkyCoord Instance Directly From the Annotations
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Copy link
Member

Choose a reason for hiding this comment

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

Can this be the -------- heading level? That would fix the docs build error

Copy link
Member

Choose a reason for hiding this comment

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

(if not I'll look into a solution)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


Once you get a ``MivotInstance`` representing the last row read, you can use it to create an ``astropy.SkyCoord`` object.

.. code-block:: python
:caption: Accessing the model view of Astropy table rows

from pyvo.mivot import MivotViewer
from pyvo.mivot.utils.dict_utils import DictUtils
Copy link
Member

Choose a reason for hiding this comment

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

why do you import DictUtils?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a trace of removed statement: corrected


m_viewer = MivotViewer(path_to_votable)
mivot_instance = m_viewer.dm_instance
print(mivot_instance.get_SkyCoord())
<SkyCoord (ICRS): (ra, dec) in deg(52.26722684, 59.94033461)
(pm_ra_cosdec, pm_dec) in mas / yr(-0.82, -1.85)>

This feature works under the condition that the annotations contain a valid instance of ``mango:EPochPosition``.
Although not a standard at the time of writing, the class structure supported by this implementation must match the figure above.
If the annotation do no contain any vali object, `None` is returned.
Copy link
Member

Choose a reason for hiding this comment

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

typo (valid)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

corrected


For XML Hackers
---------------

Expand Down
162 changes: 162 additions & 0 deletions pyvo/mivot/features/sky_coord_builder.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,162 @@
# Licensed under a 3-clause BSD style license - see LICENSE.rst
"""
Utility transforming MIVOT annotation into SKYCoord instances
"""

from astropy.coordinates import SkyCoord
from astropy import units as u
from astropy.coordinates import ICRS, Galactic, FK4, FK5


class MangoRoles:
"""
Place holder for the roles of the mango:EpochPosition roles
"""
LONGITUDE = "longitude"
LATITUDE = "latitude"
PM_LONGITUDE = "pmLongitude"
PM_LATITUDE = "pmLatitude"
PARALLAX = "parallax"
RADIAL_VELOCITY = "radialVelocity"
EPOCH = "epoch"
FRAME = "frame"
EQUINOX = "equinox"
PMCOSDELTAPPLIED = "pmCosDeltApplied"


# Mapping of the MANGO parameters on the SkyCoord parameters
skycoord_param_default = {
MangoRoles.LONGITUDE: 'ra', MangoRoles.LATITUDE: 'dec', MangoRoles.PARALLAX: 'distance',
MangoRoles.PM_LONGITUDE: 'pm_ra_cosdec', MangoRoles.PM_LATITUDE: 'pm_dec',
MangoRoles.RADIAL_VELOCITY: 'radial_velocity', MangoRoles.EPOCH: 'obstime'}

skycoord_param_galactic = {
MangoRoles.LONGITUDE: 'l', MangoRoles.LATITUDE: 'b', MangoRoles.PARALLAX: 'distance',
MangoRoles.PM_LONGITUDE: 'pm_l_cosb', MangoRoles.PM_LATITUDE: 'pm_b',
MangoRoles.RADIAL_VELOCITY: 'radial_velocity', MangoRoles.EPOCH: 'obstime'}


class SkyCoordBuilder(object):
'''
Utility generating SkyCoord instances from MIVOT annotations

- SkyCoord instances can only be built from model classes containing the minimal
set of required parameters (a position).
- In this implementation, only the mango:EpochPosition class is supported since
it proposes anything required to compute the epoch propagation which is a major use-case
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it proposes anything required to compute the epoch propagation which is a major use-case
it contains the information required to compute the epoch propagation which is a major use-case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

'''

def __init__(self, mivot_instance_dict):
'''
Constructor

parameters
-----------
- mivot_instance_dict: viewer.MivotInstance.to_dict()
Internal dictionary of the dynamic Python object generated
from the MIVOT block
lmichel marked this conversation as resolved.
Show resolved Hide resolved
'''
self._mivot_instance_dict = mivot_instance_dict
self._map_coord_names = None

def build_sky_coord(self):
"""
Build a SkyCoord instance from the dynamic Python object attribute.

returns
-------
- SkyCoord instance or None
"""
if not self._mivot_instance_dict:
return None

Check warning on line 71 in pyvo/mivot/features/sky_coord_builder.py

View check run for this annotation

Codecov / codecov/patch

pyvo/mivot/features/sky_coord_builder.py#L71

Added line #L71 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

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

could this be a return only? Any particular reason to emphasize it's None?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see comment above

if self._mivot_instance_dict["dmtype"] == "mango:EpochPosition":
return self._build_sky_coord_from_mango()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we should throw an exception instead if a user wants to get the sky coordinates but the MIVOT metadata is not available with the results. The advantage is that the message in the exception can state the issue clearly whereas None is less informative. Opinions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've no strong opinion about this.
Having no mango:EpochPosition instance in the mapping is not an error, this why I choose to return None, but I agree that using an exception allows to tell the users why he/she cannot get a SkyCoord .
If we do so, we cannot use the MivotException which is risen when something went wrong with the annotations which is not the case here.
I think that if we decide to use an exception, we should add a new one (e.g. NoMatchingClass) or use Python built-in one.

Copy link
Contributor Author

@lmichel lmichel Sep 27, 2024

Choose a reason for hiding this comment

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

After 2 days of thinking, followed by a short discussion with @ManonMarchand, who also suggested a warning, I am leaning towards implementing an exception.
The reason for this is that I bet that similar cases will occur again as long as the binding of PyVO to Mivot gets tighter. So it is wise to foresee a pattern for cases where mapped data does not cover the PYVO API expectations.

The last difficulty is find out a relevant name since the concepts of class/instance also refer to Python stuff.
I propose : NoMatchingDMType.
If none has objection, I'll implement this quickly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

return None

Check warning on line 74 in pyvo/mivot/features/sky_coord_builder.py

View check run for this annotation

Codecov / codecov/patch

pyvo/mivot/features/sky_coord_builder.py#L74

Added line #L74 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need to return a None here.

Copy link
Contributor Author

@lmichel lmichel Sep 6, 2024

Choose a reason for hiding this comment

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

Some linters (e.g. sonarqube) require method to always return something or never, no mixing is allowed. This is not the case here, I removed the return None


def _set_year_time_format(self, hk_field):
"""
Format a date expressed in year as J-year

parameters
----------
- hk_field: MIVOT instance parameter as a dict

returns
-------
- formatted string
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
parameters
----------
- hk_field: MIVOT instance parameter as a dict
returns
-------
- formatted string
Parameters
----------
hk_field : dict
MIVOT instance parameter as a dict

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

"""
return (f"j{hk_field['value']}" if hk_field["unit"] in ("yr", "year")
else hk_field["value"])

def _get_space_frame(self):
"""
Build an astropy space frame instance from the MIVOT annotations.

- Equinox are supported fot FK4/5
- Ref location are not supported

returns
-------
- Astropy space frame
"""
coo_sys = self._mivot_instance_dict["coordSys"]
equinox = None
frame = coo_sys["spaceRefFrame"]["value"].lower()

if frame == 'fk4':
self._map_coord_names = skycoord_param_default
if "equinox" in coo_sys:
equinox = self._set_year_time_format(coo_sys["equinox"])
return FK4(equinox=equinox)
return FK4()

Check warning on line 111 in pyvo/mivot/features/sky_coord_builder.py

View check run for this annotation

Codecov / codecov/patch

pyvo/mivot/features/sky_coord_builder.py#L107-L111

Added lines #L107 - L111 were not covered by tests

if frame == 'fk5':
self._map_coord_names = skycoord_param_default
if "equinox" in coo_sys:
equinox = self._set_year_time_format(coo_sys["equinox"])
return FK5(equinox=equinox)
return FK5()

Check warning on line 118 in pyvo/mivot/features/sky_coord_builder.py

View check run for this annotation

Codecov / codecov/patch

pyvo/mivot/features/sky_coord_builder.py#L118

Added line #L118 was not covered by tests

if frame == 'galactic':
self._map_coord_names = skycoord_param_galactic
return Galactic()

Check warning on line 122 in pyvo/mivot/features/sky_coord_builder.py

View check run for this annotation

Codecov / codecov/patch

pyvo/mivot/features/sky_coord_builder.py#L121-L122

Added lines #L121 - L122 were not covered by tests

self._map_coord_names = skycoord_param_default
return ICRS()

def _build_sky_coord_from_mango(self):
"""
Build silently a SlyCoord instance from the mango:EpochPosition instance.
Copy link
Member

Choose a reason for hiding this comment

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

typo (SlyCoord)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

corrected

No error is trapped, unconsistencies in the mango:EpochPosition instance will
raise Astropy errors.

- The epoch (obstime) is meant to be given as J-year.
- ICRS frame is taken by default
- The cos-delta correction is meant to be applied.
The case mango:pmCosDeltApplied = False is not suppored yet

returns
-------
- SkyCoord instance
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
returns
-------
- SkyCoord instance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

"""
kwargs = {}
kwargs["frame"] = self._get_space_frame()

for key, value in self._map_coord_names.items():
# ignore not set parameters
if key not in self._mivot_instance_dict:
continue
hk_field = self._mivot_instance_dict[key]
# format the observation time (J-year by default)
if value == "obstime":
kwargs[value] = self._set_year_time_format(hk_field)
# Convert the parallax (mango) into a distance
elif value == "distance":
kwargs[value] = (hk_field["value"]
* u.Unit(hk_field["unit"]).to(u.parsec, equivalencies=u.parallax()))
kwargs[value] = kwargs[value] * u.parsec
elif "unit" in hk_field and hk_field["unit"]:
kwargs[value] = hk_field["value"] * u.Unit(hk_field["unit"])
else:
kwargs[value] = hk_field["value"]

Check warning on line 161 in pyvo/mivot/features/sky_coord_builder.py

View check run for this annotation

Codecov / codecov/patch

pyvo/mivot/features/sky_coord_builder.py#L161

Added line #L161 was not covered by tests
return SkyCoord(**kwargs)
149 changes: 149 additions & 0 deletions pyvo/mivot/tests/test_sky_coord_builder.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
'''
The first service in operation the annotates query responses in the fly is Vizier
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The first service in operation the annotates query responses in the fly is Vizier
The first service in operation that annotates query responses on the fly is Vizier

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

https://cds/viz-bin/mivotconesearch/VizierParams
Data are mapped on the mango:EpochPropagtion class as it is implemented in the current code.
This test case is based on 2 VOTables:
Both tests check the generation of SkyCoord instances from the MivotInstances buil
Copy link
Member

Choose a reason for hiding this comment

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

typo (built)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

corrected

for the output of this service.
'''
import pytest
from pyvo.mivot.version_checker import check_astropy_version
from pyvo.mivot.viewer.mivot_instance import MivotInstance
from pyvo.mivot.features.sky_coord_builder import SkyCoordBuilder

# annotations generated by Vizier as given to the MivotInstance
vizier_dict = {
"dmtype": "mango:EpochPosition",
"longitude": {
"dmtype": "ivoa:RealQuantity",
"value": 52.26722684,
"unit": "deg",
"ref": "RAICRS",
},
"latitude": {
"dmtype": "ivoa:RealQuantity",
"value": 59.94033461,
"unit": "deg",
"ref": "DEICRS",
},
"pmLongitude": {
"dmtype": "ivoa:RealQuantity",
"value": -0.82,
"unit": "mas/yr",
"ref": "pmRA",
},
"pmLatitude": {
"dmtype": "ivoa:RealQuantity",
"value": -1.85,
"unit": "mas/yr",
"ref": "pmDE",
},
"epoch": {
"dmtype": "ivoa:RealQuantity",
"value": 1991.25,
"unit": "yr",
"ref": None,
},
"coordSys": {
"dmtype": "coords:SpaceSys",
"dmid": "SpaceFrame_ICRS",
"dmrole": "coords:Coordinate.coordSys",
"spaceRefFrame": {
"dmtype": "coords:SpaceFrame",
"value": "ICRS",
"unit": None,
"ref": None,
},
},
}
# The same edited by hand (parallax added and FK5 + Equinox frame)
vizier_equin_dict = {
"dmtype": "mango:EpochPosition",
"longitude": {
"dmtype": "ivoa:RealQuantity",
"value": 52.26722684,
"unit": "deg",
"ref": "RAICRS",
},
"latitude": {
"dmtype": "ivoa:RealQuantity",
"value": 59.94033461,
"unit": "deg",
"ref": "DEICRS",
},
"pmLongitude": {
"dmtype": "ivoa:RealQuantity",
"value": -0.82,
"unit": "mas/yr",
"ref": "pmRA",
},
"pmLatitude": {
"dmtype": "ivoa:RealQuantity",
"value": -1.85,
"unit": "mas/yr",
"ref": "pmDE",
},
"parallax": {
"dmtype": "ivoa:RealQuantity",
"value": 0.6,
"unit": "mas",
"ref": "parallax",
},
"epoch": {
"dmtype": "ivoa:RealQuantity",
"value": 1991.25,
"unit": "yr",
"ref": None,
},
"coordSys": {
"dmtype": "coords:SpaceSys",
"dmid": "SpaceFrame_ICRS",
"dmrole": "coords:Coordinate.coordSys",
"spaceRefFrame": {
"dmtype": "coords:SpaceFrame.spaceRefFrame",
"value": "FK5",
"unit": None,
"ref": None,
},
"equinox": {
"dmtype": "coords:SpaceFrame.equinox",
"value": "2012",
"unit": "yr",
},
},
}


@pytest.mark.skipif(not check_astropy_version(), reason="need astropy 6+")
def test_vizier_output():
""" Test the SkyCoord issued from the Vizier response
"""
mivot_instance = MivotInstance(**vizier_dict)
scb = SkyCoordBuilder(mivot_instance.to_dict())
scoo = scb.build_sky_coord()
assert (str(scoo).replace("\n", "").replace(" ", "")
== "<SkyCoord (ICRS): (ra, dec) in deg(52.26722684, 59.94033461) "
"(pm_ra_cosdec, pm_dec) in mas / yr(-0.82, -1.85)>")
scoo = mivot_instance.get_SkyCoord()
assert (str(scoo).replace("\n", "").replace(" ", "")
== "<SkyCoord (ICRS): (ra, dec) in deg(52.26722684, 59.94033461) "
"(pm_ra_cosdec, pm_dec) in mas / yr(-0.82, -1.85)>")


@pytest.mark.skipif(not check_astropy_version(), reason="need astropy 6+")
def test_vizier_output_with_equinox_and_parallax():
"""Test the SkyCoord issued from the modofier Vizier response *
(parallax added and FK5 + Equinox frame)
"""
mivot_instance = MivotInstance(**vizier_equin_dict)
scb = SkyCoordBuilder(mivot_instance.to_dict())
scoo = scb.build_sky_coord()
assert (str(scoo).replace("\n", "").replace(" ", "")
== "<SkyCoord (FK5: equinox=J2012.000): (ra, dec, distance) in "
"(deg, deg, pc)(52.26722684, 59.94033461, 600.) "
"(pm_ra_cosdec, pm_dec) in mas / yr(-0.82, -1.85)>")
scoo = mivot_instance.get_SkyCoord()
assert (str(scoo).replace("\n", "").replace(" ", "")
== "<SkyCoord (FK5: equinox=J2012.000): (ra, dec, distance) in "
"(deg, deg, pc)(52.26722684, 59.94033461, 600.) "
"(pm_ra_cosdec, pm_dec) in mas / yr(-0.82, -1.85)>")
Loading
Loading