Feature skycoord #591

lmichel · 2024-09-04T14:49:21Z

Extension of the MIVOT feature answering #523.

If the MIVOT annotation block contains a valid instance of the mango:EpochPosition class, the dynamic object describing the mapped data can now generate a valid SkyCoord instance.

This feature makes quite easier the epoch propagation computation with the Astropy library.

from pyvo.mivot import MivotViewer
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)>

Although not a standard at the time of writing, the class structure supported by the implementation must match the figure in the documentation:

The logic relies only on the vodml-id it finds in the annotation, without regard to the model structure; it can be easily extended to components of other models such as measurement.

The space frame implementation supports FK4/5 (with equinox), ICRS and Galactic.
The space frame standard location is not supported
Time frame are not supported
Epochs in year are processed as Jyear

codecov · 2024-09-04T14:57:18Z

Codecov Report

Attention: Patch coverage is 90.19608% with 10 lines in your changes missing coverage. Please review.

Project coverage is 81.84%. Comparing base (e0684d1) to head (ff0eeea).
Report is 135 commits behind head on main.

Files with missing lines	Patch %	Lines
pyvo/mivot/viewer/mivot_viewer.py	60.00%	4 Missing ⚠️
pyvo/mivot/features/sky_coord_builder.py	95.58%	3 Missing ⚠️
pyvo/mivot/features/static_reference_resolver.py	50.00%	1 Missing ⚠️
pyvo/mivot/utils/dict_utils.py	50.00%	1 Missing ⚠️
pyvo/mivot/utils/xml_utils.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #591      +/-   ##
==========================================
+ Coverage   81.46%   81.84%   +0.38%     
==========================================
  Files          68       70       +2     
  Lines        7034     7156     +122     
==========================================
+ Hits         5730     5857     +127     
+ Misses       1304     1299       -5

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

bsipocz · 2024-09-04T15:17:36Z

One of the test failures in unrelated and we experience it on main, too; but the other one is in mivot.

lmichel · 2024-09-04T15:34:50Z

There is a coverage issue, I'm on it

bsipocz

One tip for fixing the docs build.

bsipocz · 2024-09-04T16:04:59Z

docs/mivot/index.rst

@@ -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
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


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

(if not I'll look into a solution)

bsipocz · 2024-09-04T16:07:33Z

pyvo/mivot/features/sky_coord_builder.py

+            return None
+        if self._mivot_instance_dict["dmtype"] == "mango:EpochPosition":
+            return self._build_sky_coord_from_mango()
+        return None


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

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

bsipocz · 2024-09-04T16:08:20Z

pyvo/mivot/features/sky_coord_builder.py

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


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

see comment above

bsipocz

Thank you @lmichel for addressing the review. This looks good to me now, but I don't yet merge it to give a chance for the others to chime in.

ManonMarchand

Just saw a few typos.

Also I'm wondering why SkyCoordBuilder has to take a dict in entry and not a MivotInstance. Could it call to_dict internally and spare a call each time? Or is it meant to work on a dictionary not instantiated with a call of to_dict on a MivotInstance?

ManonMarchand · 2024-09-18T08:11:22Z

pyvo/mivot/features/sky_coord_builder.py

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


typo (SlyCoord)

ManonMarchand · 2024-09-18T08:12:52Z

pyvo/mivot/tests/test_sky_coord_builder.py

+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


typo (built)

ManonMarchand · 2024-09-18T08:17:29Z

docs/mivot/index.rst

+    :caption: Accessing the model view of Astropy table rows
+
+	from pyvo.mivot import MivotViewer
+    from pyvo.mivot.utils.dict_utils import DictUtils


why do you import DictUtils?

This was a trace of removed statement: corrected

ManonMarchand · 2024-09-18T08:17:51Z

docs/mivot/index.rst

+
+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.


typo (valid)

lmichel · 2024-09-19T09:26:45Z

Using a dict as input parameter for SkyCoordBuilder is a deliberate choice.

There were several reasons for this:

This set up a loose coupling between this class and MivotInstance and allows SkyCoordBuilder to be used out of the MivotInstance context.
In a normal use of the API, the user does not need to worry about this dictionary, as it is wrapped into the factory capability of MivotInstance:

mivot_instance.get_SkyCoord()

More practically, importing MivotInstance in sky_coord_builder.pywould lead to an import cycle.

andamian · 2024-09-23T22:50:25Z

docs/mivot/index.rst

+
+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 valid object, `None` is returned.


Suggested change

If the annotation do no contain any valid object, `None` is returned.

If the annotations do not contain any valid object, `None` is returned.

andamian · 2024-09-23T22:55:23Z

pyvo/mivot/features/sky_coord_builder.py

+    - 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


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

andamian · 2024-09-23T23:04:04Z

pyvo/mivot/features/sky_coord_builder.py

+            self._map_coord_names = skycoord_param_default
+            if "equinox" in coo_sys:
+                equinox = self._set_year_time_format(coo_sys["equinox"], True)
+                print(coo_sys["equinox"])


print? Are these leftovers from debugging?

andamian · 2024-09-23T23:11:43Z

pyvo/mivot/tests/test_sky_coord_builder.py

@@ -0,0 +1,166 @@
+'''
+The first service in operation the annotates query responses in the fly is Vizier


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

lmichel · 2024-09-24T14:38:42Z

I also updated the change log

andamian · 2024-09-24T16:28:04Z

pyvo/mivot/features/sky_coord_builder.py

+        - SkyCoord instance or None
+        """
+        if self._mivot_instance_dict and self._mivot_instance_dict["dmtype"] == "mango:EpochPosition":
+            return self._build_sky_coord_from_mango()


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?

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.

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

bsipocz

OK, so I started to give this a final review and found the following, albeit nitpicky things:

docstrings should be in numpydocs format. Since there wasn't that terribly many of them I left all the fixes as code suggestions.
in practice exception and warning classes have Error and Warning in their name. Thus please use NoMatchingDMTypeError. I started adding comments for these, but it's
way to error-prone on github, so I haven't done it for the full code diff.
I would also say to have it as a subclass of TypeError or ValueError, but I can be convinced otherwise.

I leave the Affect-dev label on, so please ignore any changelog check failures. This label will help me as a reminder at release time to condense down the entries to be the most useful for the users.

Once the first two are addressed, I think this should go ahead and be merged.

pyvo/mivot/features/sky_coord_builder.py

bsipocz · 2024-09-30T21:11:20Z

pyvo/mivot/features/sky_coord_builder.py

+        returns
+        -------
+        - SkyCoord instance
+
+        raises
+        ------
+        - NoMatchingDMType: if the SkyCoord instance cannot be built.


Suggested change

returns

-------

- SkyCoord instance

raises

------

- NoMatchingDMType: if the SkyCoord instance cannot be built.

Raises

------

NoMatchingDMTypeError

If the SkyCoord instance cannot be built.

bsipocz · 2024-09-30T21:12:28Z

pyvo/mivot/features/sky_coord_builder.py

+        raise NoMatchingDMType("No INSTANCE with dmtype='mango:EpochPosition' has been found:"
+                               " cannot build a SkyCoord from annotations")


Suggested change

raise NoMatchingDMType("No INSTANCE with dmtype='mango:EpochPosition' has been found:"

" cannot build a SkyCoord from annotations")

raise NoMatchingDMTypeError("No INSTANCE with dmtype='mango:EpochPosition' has been "

"found: cannot build a SkyCoord from annotations")

bsipocz · 2024-09-30T21:13:11Z

pyvo/mivot/tests/test_sky_coord_builder.py

+    """
+    Test that a NoMatchingDMType is raised not mapped on mango:EpochPosition
+    """
+    with pytest.raises(NoMatchingDMType):


Suggested change

with pytest.raises(NoMatchingDMType):

with pytest.raises(NoMatchingDMTypeError):

bsipocz · 2024-09-30T21:13:54Z

pyvo/mivot/tests/test_sky_coord_builder.py

+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
+from pyvo.mivot.utils.exceptions import NoMatchingDMType


Suggested change

from pyvo.mivot.utils.exceptions import NoMatchingDMType

from pyvo.mivot.utils.exceptions import NoMatchingDMTypeError

bsipocz · 2024-09-30T21:23:45Z

pyvo/mivot/features/sky_coord_builder.py

+        parameters
+        ----------
+        - hk_field: MIVOT instance parameter as a dict
+
+        returns
+        -------
+        - formatted string


Suggested change

parameters

----------

- hk_field: MIVOT instance parameter as a dict

returns

-------

- formatted string

Parameters

----------

hk_field : dict

MIVOT instance parameter as a dict

bsipocz · 2024-09-30T21:25:26Z

pyvo/mivot/features/sky_coord_builder.py

+        parameters
+        ----------
+        - obstime: string
+                   Observation time is given here because KF4 set an default value
+                   if it is not given
+        returns
+        -------
+        - Astropy space frame


Suggested change

parameters

----------

- obstime: string

Observation time is given here because KF4 set an default value

if it is not given

returns

-------

- Astropy space frame

Parameters

----------

obstime : str

Observation time is given here because KF4 set an default value

if it is not given

Returns

-------

Astropy space frame

bsipocz · 2024-09-30T21:26:20Z

pyvo/mivot/features/sky_coord_builder.py

+        returns
+        -------
+        - SkyCoord instance


Suggested change

returns

-------

- SkyCoord instance

bsipocz · 2024-09-30T21:27:00Z

pyvo/mivot/viewer/mivot_instance.py

+        returns
+        -------
+        - a SkyCoord instance or None


Suggested change

returns

-------

- a SkyCoord instance or None

Returns

-------

SkyCoord instance or None

bsipocz · 2024-09-30T21:27:28Z

pyvo/mivot/viewer/mivot_instance.py

@@ -128,6 +125,14 @@ def update(self, row, ref=None):
                    setattr(self, self._remove_model_name(key),
                            MivotUtils.cast_type_value(row[ref], getattr(self, 'dmtype')))

+    def get_SkyCoord(self):


Does this have to be a public method, are the users expected to run it?

This is the public method the must be run by users. The SkyCoordBuilder class is that one that is hidden for the users

explicit

- MappingException renamed as MappingError and MivotException as MivotError - NoMatchingDMTypeError typed as child of TypeError. - Both MappingError and MivotError keep children of Exception.

lmichel · 2024-10-01T09:58:11Z

After the last review I did the following changes

Docstrings of sky_coord_builder.py have been reformatted according to the numpy style, including the return statements that ware not mentioned in the review.
Some doctring content has been made more explicit
NoMatchingDMType as been renamed as NoMatchingDMTypeError and set as child of TypeError
I've also renamed MivotException and MappingException with a trailing Error (commit 7b635a34958404fb3000b88945509f098f6d07d7). As this concern most of package files, the commit can be reverted to save review time

bsipocz

Thank you @lmichel for addressing all the comments! I'll go ahead and merge this now, if anything else comes up please open a follow-up issue/PR.

lmichel added 2 commits September 4, 2024 16:21

Add the SkyCoord generation from the Mivot object

0f41497

remove unused attribute

df9aa22

bsipocz added this to the v1.6 milestone Sep 4, 2024

bsipocz added the component: mivot label Sep 4, 2024

bsipocz reviewed Sep 4, 2024

View reviewed changes

lmichel added 2 commits September 6, 2024 09:50

Fix for the PR

7b906fb

Fix for the PR

a8c7624

bsipocz added the Affects-dev To be used for follow-up PRs for new engancements or bugs only present on dev label Sep 6, 2024

bsipocz approved these changes Sep 6, 2024

View reviewed changes

ManonMarchand reviewed Sep 18, 2024

View reviewed changes

lmichel added 2 commits September 19, 2024 17:43

Typos in comment corrected (astropy#591)

094b519

Typos corrected and unused import removed (astropy#591)

0afbd2a

andamian reviewed Sep 23, 2024

View reviewed changes

Damian's review + change log

e0f61e7

fix CI

aad09be

andamian reviewed Sep 24, 2024

View reviewed changes

lmichel added 4 commits September 30, 2024 09:19

raise a exception (NoMatchingDMType) when the API miss a Model object

833d9f1

Damian's review + change log

9a7263f

fix CI

cfdac89

rebase recent history

ff738ee

ManonMarchand removed the Affects-dev To be used for follow-up PRs for new engancements or bugs only present on dev label Sep 30, 2024

bsipocz requested changes Sep 30, 2024

View reviewed changes

bsipocz added the Affects-dev To be used for follow-up PRs for new engancements or bugs only present on dev label Sep 30, 2024

lmichel added 2 commits October 1, 2024 09:28

NoMatchingDMType renamed as NoMatchingDMTypeError

c614ee4

Make docstrings compliant with the numpy style, make some comments more

f60533b

explicit

Rename Exceptions

ff0eeea

- MappingException renamed as MappingError and MivotException as MivotError - NoMatchingDMTypeError typed as child of TypeError. - Both MappingError and MivotError keep children of Exception.

bsipocz approved these changes Oct 1, 2024

View reviewed changes

bsipocz merged commit 9d3c5be into astropy:main Oct 1, 2024
12 of 13 checks passed

	If the annotation do no contain any valid object, `None` is returned.
	If the annotations do not contain any valid object, `None` is returned.

	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

		@@ -0,0 +1,166 @@
		'''
		The first service in operation the annotates query responses in the fly is Vizier

	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

		raise NoMatchingDMType("No INSTANCE with dmtype='mango:EpochPosition' has been found:"
		" cannot build a SkyCoord from annotations")

	with pytest.raises(NoMatchingDMType):
	with pytest.raises(NoMatchingDMTypeError):

	from pyvo.mivot.utils.exceptions import NoMatchingDMType
	from pyvo.mivot.utils.exceptions import NoMatchingDMTypeError

Feature skycoord #591

Feature skycoord #591

Conversation

lmichel commented Sep 4, 2024

codecov bot commented Sep 4, 2024 • edited Loading

Codecov Report

bsipocz commented Sep 4, 2024

lmichel commented Sep 4, 2024

bsipocz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmichel Sep 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsipocz left a comment

Choose a reason for hiding this comment

ManonMarchand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmichel commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmichel commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmichel Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsipocz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmichel commented Oct 1, 2024

bsipocz left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 4, 2024 •

edited

Loading

lmichel Sep 6, 2024 •

edited

Loading

lmichel Sep 27, 2024 •

edited

Loading