Mango next by lmichel · Pull Request #698 · astropy/pyvo

lmichel · 2025-09-04T13:55:43Z

This MR only relates to the annotation readout.

Code cleaning:

Unused module XmlViewer + dependencies has been removed
Glossary has been cleaned up

MivotViewer:

Able to process TEMPLATES with multiple INSTANCES. This is necessary for those who want to use MANGO in sparse mode (see the spec)
Give access to global INSTANCEs as MivotInstances

This PR improves the current API. It will be extended with model specific modules once MANGO will be a REC. At that time new MANGO-specific features will be completed.

codecov · 2025-09-04T13:57:38Z

Codecov Report

❌ Patch coverage is 90.16393% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.03%. Comparing base (e892b23) to head (76a9ce9).
⚠️ Report is 81 commits behind head on main.

Files with missing lines	Patch %	Lines
pyvo/mivot/features/sky_coord_builder.py	86.56%	9 Missing ⚠️
pyvo/mivot/viewer/mivot_viewer.py	95.45%	2 Missing ⚠️
pyvo/mivot/viewer/mivot_instance.py	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #698      +/-   ##
==========================================
- Coverage   84.09%   84.03%   -0.07%     
==========================================
  Files          80       79       -1     
  Lines        8556     8529      -27     
==========================================
- Hits         7195     7167      -28     
- Misses       1361     1362       +1

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

bsipocz

Some minor nitpicking comments

bsipocz · 2025-10-08T13:31:03Z

        sparse: boolean, optional (default to False)
            If True, all properties are added in a independent way to the the TEMPLATES.
            They are packed in a MangoObject otherwise.
+        schema_check: boolean, optional (default to True)


We need to run a docs format checker (to be done separately from this PR), this is just a reminder mostly to myself to do it (these white spaces affect the actual API docs rendering)

Suggested change

schema_check: boolean, optional (default to True)

schema_check : boolean, optional (default to True)

Fixed, but I found out tens of occurrences of this pattern (/[a-z]: /) in many files. I prefer to have your green light before to increase the number of modified files. Could this be done in a specific PR?

yes, let's not mix those changes into this PR but do it separately, and I'll also try to remember to pick a tool that does that linting for us.

I'll open a dedicated PR once this one will be merged.

bsipocz · 2025-10-08T15:23:30Z

-
-
-@prototype_feature('MIVOT')
-class XMLViewer:


This was a prototype, so I suppose changing the API without any prior notice is fine and thus this can be removed without deprecation. But this class was mentioned in the narrative docs, so it's just something to be aware of.

made it more explicit in the change log

bsipocz · 2025-10-08T15:27:57Z

+            raise MivotError(
+                "Can't find " + Ele.INSTANCE
+                + " in " + Ele.TEMPLATES
+            )


absolute nitpicking, but I feel this could all fit in one line; we're quite liberal with line lengths and allow them to be up to 110 characters

tomdonaldson

Thanks very much for this @lmichel! It's very exciting to see this working so cleanly with real services. Other than the minor comments I left, I'm very comfortable merging this to make it available for people to try out.

I left a number of minor comments, mostly coming from minor issues I encountered while working through viewer.rst and example.rst. Please have a look and address the ones that make sense to you. If any of the comments make sense but are difficult to resolve, then they can be left for later, because it will be good to have this functionality merged.

tomdonaldson

Thanks for the updates @lmichel. This looks great to me. I'll let @bsipocz take a last look before merging.

bsipocz

I see some of Tom's comments haven't got addressed (they are kind of hidden under a "hidden conversation" button. I assume you haven't seen them and they got just accidentally left out. I'm happy to work them in, as I plan to rebase before merge anyway due to a duplicated commit in the history.
Let me know if my read of the situation is correct, so I'll go ahead with the minor fixes and merge this.

bsipocz · 2025-11-06T02:01:37Z

   print(mivot_instance.__dict__.keys())
-   dict_keys(['dmtype', 'longitude', 'latitude', 'pmLongitude', 'pmLatitude', 'epoch', 'Coordinate_coordSys'])
+   # dict_keys(['dmtype', 'longitude', 'latitude', 'pmLongitude', 'pmLatitude', 'epoch', 'Coordinate_coordSys'])

   print(mivot_instance.Coordinate_coordSys.__dict__.keys())
-   dict_keys(['dmtype', 'dmid', 'dmrole', 'spaceRefFrame'])
+   # dict_keys(['dmtype', 'dmid', 'dmrole', 'spaceRefFrame'])

   print(mivot_instance.Coordinate_coordSys.spaceRefFrame.__dict__.keys())
-   dict_keys(['dmtype', 'value', 'unit', 'ref'])
+   # dict_keys(['dmtype', 'value', 'unit', 'ref'])


why are these commented out?

Tom suggested this to allows users to cut/paste working code snippet

If we add these as code examples for doctest, then the copybutton sphinx extension (>>> arrows on the top right corner of the box) takes care of ripping out the print results.
Try this one: https://pyvo.readthedocs.io/en/latest/mivot/viewer.html

So, overall I would rather suggest to have these formatted with the >>>s included

I replaced all .. code-block with .. doctest-skip::

tomdonaldson · 2025-11-06T17:48:12Z

I ran through the doc code again and am quite happy with it. I did just push one minor fix where dm_global_instances was being accessed as a function (with parentheses) instead of as a property.

andamian · 2025-11-06T22:32:08Z

+   for globals_instance in m_viewer.dm_globals_instances:
+       print(globals_instance)
+
+.. code-block:: json


Why json and not Python dictionary? json should be a serialization method.

The json flag was here to tell Sphinx how to highlight the syntax.
I replaced all code-block with .. doctest-skip:: as suggested bu @bsipocz anyway

No, it's not about docs it's about the format that print(globals_instance) produces. I expected to be something more concise so that the user would be able to do log.debug("Process globals_instance: " + globals_instance) or similar and have it show up in a single row.

I see.
I put a single line string representation (<MivotInstance: dmtype="mango:EpochPosition">) for MivotInsance.
The whole representation (dictionary) is now only used by repr()

andamian · 2025-11-06T22:48:59Z

+            Python object generated from the MIVOT block as either a Pyhon object or a dict
        '''
-        self._mivot_instance_dict = mivot_instance_dict
+        if isinstance(mivot_instance, dict):


Again, no point for 2 alternative types for model representation. End users are interested in actual MANGO or other astronomy related objects (SkyCoords) not the model representation. Just my opinion.

this is mostly driven by the will of simplifying unit tests

This feature has been removed

andamian · 2025-11-07T21:24:34Z

+   >>> 
+   >>> # Print out the json serialization of the Python object
+   >>> print(mango_property)
+   {


My suggestion is to not expose the model representation to the user. I don't recall but it must be a way to either specifically request MANGO model classes or to discover the ones that are available. The "discovery" mechanism can be either an explicit "get all the types" or simply trying, e.g. check wether getSkyCoord(row) which looks for the dm_instance.dm_type=mango:EpochPosition and returns the corresponding sky coordinates or null otherwise. But the mangoProperty is just a serialization representation.

andamian · 2025-11-07T21:44:32Z

+                    self._mivot_instance_dict = mango_property
+                    return self._build_sky_coord_from_mango()
+            raise NoMatchingDMTypeError(
+                "No INSTANCE with dmtype='mango:EpochPosition' has been found:"


Or simply return None. I'm not sure which one is better as a discovery mechanism.

honestly, I've no preference. I can change this if you wish.
Using exception:

PRO: Give a clear view on the reason of the failure

CON: Break the current execution thread

andamian · 2025-11-07T22:13:04Z

+    <VODML xmlns="http://www.ivoa.net/xml/mivot">
+        <REPORT status="OK"/>
+        <MODEL name="ivoa" url="https://www.ivoa.net/xml/VODML/IVOA-v1.vo-dml.xml"/>
+        <MODEL name="coords" url="https://ivoa.net/xml/VODML/Coords-v1.vo-dml.xml"/>


https://ivoa.net/xml/VODML/Coords-v1.0.vo-dml.xml - the correct URL

andamian · 2025-11-09T18:10:56Z

+    scb = SkyCoordBuilder(mivot_instance)
+    scoo = scb.build_sky_coord()
+
+    assert (str(scoo).replace("\n", "").replace("  ", "")


This is fragile subject to astropy changing their representation format. It should check the values instead.

bsipocz

Some more minor comments, they are not blockers or really actionable here, but we may nevertheless sort them out in a follow-up -- e.g. about the skipped tests.

Also, there is a failure on the devtest job, I'll have a look at that before merging this (aiming by this evening (CET time) and cutting a release (aiming tomorrow evening or on Wednesday the latest)).

(and will rebase to clean up the duplicated commits -- again, no action items for you there).

bsipocz · 2025-11-10T07:53:03Z

+
+.. doctest-skip::
+
+    >>> import pytest


I don't think we should have any pytest machinery for skipping in the narrative docs examples, it could be said at the top that one needs a x.y astropy to run these examples (and as they are skipped anyway it won't be an issue for out actual testing (or also we can use doctest requires at the top of the files.)

bsipocz · 2025-11-10T07:58:10Z

+   >>> from pyvo.dal.scs import SCSService
+   >>> from pyvo.utils import activate_features
+   >>> from pyvo.mivot.viewer.mivot_viewer import MivotViewer
+   >>> from pyvo.mivot.features.sky_coord_builder import SkyCoordBuilder


This is not an issue at all, but I would think that in narrative documentation there is a benefit of doing the imports at piecemeal -- only in the cells where they are (first) used, where the classes/functions are being first described as opposed to doing everything at the top.

agree, to be fixed later

updated yet)

TEMPLATES)

To be continued accordingly to the MANGO recommandation process

to access remote resources

Glossary - improve the test coverage

with local resources only

…) or from a MivotInstance

annotated Vizier CS + a few other minor cosmetics

doctest in example.rst

one (whole dictionary) is still in use as a repr()

representation

of the current PR (+flake8)

bsipocz · 2025-11-10T12:16:25Z

Thank you @lmichel!

lmichel · 2025-11-10T16:39:30Z

A big thank you to the reviewer team

lmichel force-pushed the mango-next branch from b4601e5 to cf8e572 Compare September 25, 2025 15:30

bsipocz added component: mivot enhancement labels Sep 25, 2025

bsipocz added this to the v1.8 milestone Sep 25, 2025

lmichel marked this pull request as ready for review October 8, 2025 08:51

bsipocz reviewed Oct 8, 2025

View reviewed changes

tomdonaldson reviewed Oct 30, 2025

View reviewed changes

tomdonaldson approved these changes Nov 4, 2025

View reviewed changes

bsipocz approved these changes Nov 6, 2025

View reviewed changes

andamian reviewed Nov 6, 2025

View reviewed changes

andamian approved these changes Nov 9, 2025

View reviewed changes

bsipocz reviewed Nov 10, 2025

View reviewed changes

bsipocz force-pushed the mango-next branch from 9e28d1d to d74f09f Compare November 10, 2025 11:58

lmichel added 15 commits November 10, 2025 03:59

remove obsolete words

1cf1084

Remove XML API which has never been used

29a5e1d

update docstrings and process the when no TEMPLATES is found

37045b9

support the case of TEMPLATES containing multiple INSTANCE (doc not

8af04b9

updated yet)

adapt the test suite to the new viewer feature (multiple INSTANCE in

d66808a

TEMPLATES)

The viewer builds MivotInstances for INSTANCEs children of GLOBALS

8b50d62

add missing astropy version checking

12d4a7b

make SkyCoordBuilder able to process MangoObject instances

4a0520f

To be continued accordingly to the MANGO recommandation process

minor update to work with the Simbad output conesearch

bff0679

make the MIVOT schema validation optional in order to prevent unit tests

40d6c3b

to access remote resources

fix a big mistake in the parallax to distance computation

428e023

mapping EpochPostion to SkyCoord moved from SkyCoordBuilder to the

d2f4e28

Glossary - improve the test coverage

Use of local copies of VOTable schemas to make XML validation working

fd70032

with local resources only

update doc strings- remove useless methods - make some minor fixes

37ebd9a

SkyCoordBuilder can be instantiated from either a dict (for the tests…

5efd300

…) or from a MivotInstance

lmichel and others added 22 commits November 10, 2025 03:59

flake8 - revamp the viewer documentation

355e0ad

changelog updated

ebfe70b

add homework with Simbad

f3bd8d6

Correct doc style a little bit

0185438

Some style improvement by DeepL

8647023

Brigita's review (2025-10-08)

204e961

compress repeated code

2d6be69

Tom's review (code snippet fixes, various typos) + release date of

a6bb26f

annotated Vizier CS + a few other minor cosmetics

skipped tom's issues

e1b9c5d

skipped tom's comment

e0baff3

fix CI failures

a0da9f0

Remove parens since dm_global_instances is a property.

b55ea05

SkyCoordBuilder constructor no longer accepts a dict as input parameter

3abbc9b

replace code_blocks with doctest

bc66970

remove reference to the dual representation and replace code_blocks with

5785998

doctest in example.rst

add a single line string representation of MivotInstance. The former

e9339cf

one (whole dictionary) is still in use as a repr()

mention the way to discover the mapped instances

6443e7b

fix the coords URL

53b3edd

highlight the discovery of the mapped instances

b690e3d

valid SKyCoord instance field by field instead of comparing the string

49ed68f

representation

fix the case where no ucd is provided. This bugfix is out of the scope

3a6690b

of the current PR (+flake8)

fix required astropy version

735c233

bsipocz force-pushed the mango-next branch from d74f09f to 4d5dbbf Compare November 10, 2025 11:59

remove astropy check from code snippet

76a9ce9

bsipocz force-pushed the mango-next branch from 4d5dbbf to 76a9ce9 Compare November 10, 2025 12:13

bsipocz merged commit 9ecff0c into astropy:main Nov 10, 2025
14 checks passed

McSinyx mentioned this pull request Feb 2, 2026

Install VOTable schemas #723

Merged

lmichel deleted the mango-next branch April 17, 2026 16:14

	schema_check: boolean, optional (default to True)
	schema_check : boolean, optional (default to True)

Uh oh!

Uh oh!

Conversation

lmichel commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

This MR only relates to the annotation readout.

Uh oh!

codecov Bot commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmichel Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tomdonaldson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tomdonaldson left a comment

Choose a reason for hiding this comment

Uh oh!

bsipocz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomdonaldson commented Nov 6, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmichel Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

lmichel commented Sep 4, 2025 •

edited

Loading

codecov Bot commented Sep 4, 2025 •

edited

Loading

lmichel Oct 9, 2025 •

edited

Loading

lmichel Nov 7, 2025 •

edited

Loading