Mivot writer #627

lmichel · 2024-11-30T14:03:47Z

API for creating and writing MIVOTY annotation

#. Create individual instances (INSTANCE) using the MivotInstance class
#. Wrap the annotations with the MivotAnnotations class
#. Insert the annotations into a VOtable by using the Astropy API (wrapped in the package logic).

The annotation builder does not check whether the XML conforms to any particular model.
It simply validates it against the MIVOT XML Schema if the xmlvalidator package if is installed.

The documentation has been split in 2 separate sections (reading and writing MIVOT)

for the writer

codecov · 2024-12-02T06:48:56Z

Codecov Report

Attention: Patch coverage is 86.74033% with 24 lines in your changes missing coverage. Please review.

Project coverage is 82.93%. Comparing base (f765128) to head (01b6a5c).
Report is 15 commits behind head on main.

Files with missing lines	Patch %	Lines
pyvo/mivot/writer/annotations.py	82.11%	22 Missing ⚠️
pyvo/mivot/utils/xml_utils.py	75.00%	1 Missing ⚠️
pyvo/mivot/writer/instance.py	98.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #627      +/-   ##
==========================================
+ Coverage   82.30%   82.93%   +0.63%     
==========================================
  Files          72       75       +3     
  Lines        7430     7658     +228     
==========================================
+ Hits         6115     6351     +236     
+ Misses       1315     1307       -8

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

Forced pull after having squashed tiny commits dues to CI errors

bsipocz

Some comments mostly focusing on documentation and on how the API is exposed.

bsipocz · 2024-12-06T02:21:07Z

docs/mivot/index.rst

+*********************
+MIVOT (``pyvo.mivo``)
+*********************


typo

Suggested change

*********************

MIVOT (``pyvo.mivo``)

*********************

**********************

MIVOT (``pyvo.mivot``)

**********************

bsipocz · 2024-12-06T02:31:16Z

pyvo/mivot/writer/annotations.py

+blocks into VOTable files. The MIVOT block, represented as an XML structure, is used for
+data model annotations in the IVOA ecosystem.
+
+Features:


While I see the motivation for including some narrative docs with the source code, this section is not at all included in the rendered documentation as is not being part of any testing. So any information listed here likely need to be repeated and then kept in sync with the narrative docs. So my suggestion would be to remove the details from here and only have the narrative docs, as well as docstrings in any user facing functionality.

docs/mivot/index.rst

pyvo/mivot/writer/instance.py

pyvo/mivot/utils/xml_utils.py

CHANGES.rst

pyvo/mivot/writer/annotations.py

back quote)

MIVOT items put together.

default pyvo.mivot content code stye add license

optional with mandatory keyword

lmichel · 2024-12-15T16:13:10Z

The 3.11 step failed on a Simbad TAP server error
I do not understand what the issue is with readthedoc, I guess it is a project-wide scope.

bsipocz · 2024-12-16T17:40:39Z

The simbad failure is unrelated and I see it in main, too. However the RTD is coming from this PR, I've added a patch to fix it.

This is ready for review for @astropy/pyvo-maintainers

ManonMarchand · 2025-01-06T12:08:34Z

(on the current SIMBAD list of tables failure, the new table mes_otype is currently being tested by CDS's users to see if they find it practical. It might stay or be removed in the next months depending on their feedback)

andamian · 2025-01-21T00:23:30Z

docs/mivot/index.rst


-This module contains the new feature of annotations in VOTable.
+This module contains the new feature handling model annotations in VOTable.


This sounds better IMO.

Suggested change

This module contains the new feature handling model annotations in VOTable.

This module contains the new feature of handling model annotations in VOTable.

andamian · 2025-01-21T23:17:51Z

CHANGES.rst

 - MIVOT module: If the MIVOT annotation block contains a valid instance of the
  ``mango:EpochPosition`` class, the dynamic object describing the mapped
  data can generate a valid SkyCoord instance. [#591]

+- RegTAP constraints involving tables other than rr.resource are now


I'm wondering what happened here since this is not your change.

I do not understand the issue, this relates to 1.6.

I'm wondering what happened here since this is not your change.

a rebase or merge conflict resolution went wrong

andamian · 2025-01-21T23:37:33Z

docs/mivot/viewer.rst

+------------------
+The ``ModelViewer`` module manages access to data mapped to a model through dynamically
+generated objects (``MivotInstance``class).
+The example below shows how can be consumed a VOTable resulting from a cone-search query which data are mapped


Suggested change

The example below shows how can be consumed a VOTable resulting from a cone-search query which data are mapped

The example below shows how a VOTable result of a cone-search query can be parsed and data mapped

andamian · 2025-01-21T23:45:40Z

docs/mivot/viewer.rst

+        	xml_view = mivot_viewer.xml_view
+        	# do whatever you want with this XML element
+
+It to be noted that ``mivot_viewer.xml_view`` is a shortcut


Suggested change

It to be noted that ``mivot_viewer.xml_view`` is a shortcut

It is to be noted that ``mivot_viewer.xml_view`` is a shortcut

andamian · 2025-01-22T00:41:00Z

docs/mivot/writer.rst

+        value="BARYCENTER",
+    )
+    space_frame.add_instance(ref_position)
+    space_sys.add_instance(space_frame)


I feel like this is too low level API for it to gain traction. My idea of this (as I probably mentioned to you in the past) would be to have the supported models part of this library. Users only have to define the mapping to the column names, maybe the units and the global values and a utility generates the corresponding json that goes into the header and the rest of the result content is streamed from the database. The utility can take an optional VOTable header to check if the columns in the mapping exist in the table. But all the other details (dmrole, dmtype, etc) are internal to the Model classes and not exposed to the user.

I would have preferred a simple example of a writer for SkyCoords where a utility requires the mapping of SkyCoords fields to column names but can generate the other details the MIVOT uses. Can that be a goal? Is it possible to implement?

Providing a user-friendly API is the final goal of this work but it requires a low-level API allowing to build annotations step by step which is the object of this PR.

I've another PR on the track allowing to add in one callback time frames, space frames and photometric filters, defined as instances of classes of Coordinates DM and PhotDM respectively.

I propose to first merge this incoming work with #627, but this not enough to build annotations on the fly as you suggest.
I'll add another endpoint(s) allowing to add MANGO objects even if MANGO is not a REC yet. I assume that possible requested changes won't alter too much the code.

I propose to start with the 6 parameters EpochPosition, the Brightness (mag or flux) and the Color.

The API will look like:

votable = parse(votable_path) annotations = Annotation() annotations.add_epoch_propagation(sky_position, space_frame, time_frame, correlation, errors) # something similar for the magnitude/color annotations.build_mivot_block() annotations.insert_into_votable(votable)

the parameters of add_epoch_propagation are basically dictionaries containing tuples connecting model leaves with columns identifiers or literal values.

this API could be extended with an automatic column discovery at least for columns having unambiguous UCDs

This work will take a bit of time however.
If you agree, the PR can be frozen meanwhile it is merged with that future code.

Thanks. It makes sense. Maybe just mention in the documentation that this is intended for developers of DM classes that want them to be annotated to VOTables and not for the end users.

…ivot-writer

bsipocz

@andamian - I believe all your comments have been addressed. Do you think it's good to go now?

When it gets the green light, I'll go ahead and rebase to get rid of the merge commits and the weirdness in the changelog.

bsipocz · 2025-02-15T04:04:03Z

CHANGES.rst

 - MIVOT module: If the MIVOT annotation block contains a valid instance of the
  ``mango:EpochPosition`` class, the dynamic object describing the mapped
  data can generate a valid SkyCoord instance. [#591]

+- RegTAP constraints involving tables other than rr.resource are now


I'm wondering what happened here since this is not your change.

a rebase or merge conflict resolution went wrong

bsipocz · 2025-02-15T04:19:26Z

pyvo/mivot/writer/annotations.py

+try:
+    from astropy.io.votable.tree import MivotBlock
+except ImportError:
+    pass


maybe add a comment here that this try/except should go away once we don't need to do check_astropy_version and require astropy >=6.0

andamian

Sorry. I've had these comments on Pending - just forgot to submit review. I'm doing it now.

andamian · 2025-01-26T21:27:27Z

pyvo/mivot/__init__.py

@@ -1,2 +1 @@
-# package entry point
-from .viewer.mivot_viewer import MivotViewer


Is this because MivotViewer is not part of the user facing API and just meant for library developers?

I now have 2 different features (read and write). I thought it was better to put the default imports at sub-package level (viewer, writer).
I took example on pyvo.io.
Is that not correct?

I let it as it is

andamian · 2025-01-27T02:43:17Z

pyvo/mivot/writer/annotations.py

+
+    Attributes
+    ----------
+    _models : dict


These are all private attributes. Document them online when they are declared maybe?

andamian · 2025-01-27T04:07:09Z

pyvo/mivot/writer/annotations.py

+        """
+        return self._mivot_block
+
+    def _get_report(self):


Shouldn't all MIVOT components represented by classes should have _to_xml methods to get their representations? It might be more consistent and easier to remember. Just a suggestion.

This method just returns one component of the MIVOT block: <REPORT...\> as do the following ones for MODELS/GLOBALS/TEMPLATES.
_to_xml is used for model components such as <INSTANCE>
These methods are used internally to pack the MIVOT block.
I can add add an _xml in their names (e.g. _get_xml_report) if you think it will make the code more clear but in my opinion this not a good option because these methods return strings not XML elements (etree).

I let it as it is.

andamian · 2025-01-27T04:08:50Z

pyvo/mivot/writer/instance.py

@@ -0,0 +1,160 @@
+# Licensed under a 3-clause BSD style license - see LICENSE.rst
+"""
+MivotInstance is a simple API for building MIVOT instances step by step.


Can you detail in the description what a MIVOT instance is?

andamian · 2025-01-27T04:19:42Z

pyvo/mivot/writer/instance.py

+            Free identifier of the instance
+
+    """
+    def __init__(self, dmtype=None, *, dmrole=None, dmid=None):


If dmtype is required it shouldn't have a default.

andamian · 2025-01-27T04:23:08Z

pyvo/mivot/writer/instance.py

+        """
+        if not dmtype:
+            raise MappingError("Cannot build an instance without dmtype")
+        self._dmtype = dmtype


I invite the others to chime in but I've started using a rule that we are using in Java. If the ctor argument is required, then the corresponding class attribute is read-only and uses a property to enforce that. If it's optional, then it's a read/write attribute and the attribute should be public. In the case above:

def __init__(self, dmtype, *, dmrole=None, dmid=None): self._dmtype = dmtype # check dmtype is correct self.dmrole = droll self dmid = dmid @property def dmtype(self): return self._dmtype ''' What do you think?

I'm using this pattern for read-only properties (e.g. MivotAnnotation.mivot_block) but in this case, all the class attributes are set at instantiation time are not meant to changed later or even read from outside (apart of the tests).
Do you think it worth it to add getters for them?

Probably not. Only for those that the caller might need access to.

I let it as it is

lmichel added 18 commits November 24, 2024 17:22

add a clean ns option for the pretty printing

89d8dae

layout changed after a read/write cycle

5fddfcd

local copy of the MIVOT XML schema

89c6fe3

annotation builder: new class

cf7fc92

instance builder: new class

db49366

test suite for the annotations builders

4803139

API for writing MIVOT annotations

34aacff

test suite for the MIVOT writer

1b6197b

Doc updated with the split into 2 documents, one for the reader and one

f367116

for the writer

Changelog updated

7a9cff4

code style

c8b8549

style corrections

3ad6f43

add PR number in change log

79c01b8

removed

9e20fd3

add PR number

55cc826

resolve conflicts

a622b32

add writer to api index

be59549

Merge branch 'main' into mivot-writer

c68dfaa

lmichel closed this Nov 30, 2024

fix CI

f9917ed

lmichel reopened this Dec 1, 2024

lmichel added 3 commits December 1, 2024 20:29

Merge branch 'mivot-writer' of github.com:lmichel/pyvo into mivot-writer

c7cfe7b

fix use of mapping exception

7e0c918

add package init

4b40de5

lmichel closed this Dec 1, 2024

lmichel added 3 commits December 1, 2024 20:46

fix call to MivotInstance dict

c0068aa

typo

e9e273e

test MivotViewer.to_dict() output

28ee3ed

lmichel reopened this Dec 2, 2024

flake8

9277c03

lmichel marked this pull request as ready for review December 2, 2024 17:18

lmichel added 2 commits December 3, 2024 13:08

Fix CI errors

b2efc78

Merge branch 'mivot-writer' of github.com:lmichel/pyvo into mivot-writer

2dee869

Forced pull after having squashed tiny commits dues to CI errors

bsipocz added this to the v1.7 milestone Dec 6, 2024

bsipocz added enhancement component: mivot labels Dec 6, 2024

bsipocz reviewed Dec 6, 2024

View reviewed changes

lmichel added 10 commits December 13, 2024 07:45

restructuring documentation : toc updated, exposed API

d36398b

Fix code blocks in docstring that were tagged as sphinx reference (one

1b5f460

back quote)

remove narrative doc from source files

514b48b

replace abbreviation with explicit name (clean_ns)

b79706c

add explicit definition of the exposed namespaces (__all__)

4932477

add license

b8021e9

MIVOT items put together.

Make optional parameters mandatory keywords.

0303a85

import viewer explicitely because pyvo.mivot.viewer is no longer the

3d9dec5

default pyvo.mivot content code stye add license

XML typography more homogeneous, parameter model_url of add_model made

bff20a5

optional with mandatory keyword

fix CI errors

bab58f2

DOC: fix headings

d470071

lmichel added 2 commits January 21, 2025 07:52

mv astropy#627 to 1.7 section

c39489b

Merge branch 'main' into mivot-writer

3e9c6a4

andamian reviewed Jan 22, 2025

View reviewed changes

lmichel added 2 commits January 23, 2025 09:13

astropy#627 review

f030f35

Merge branch 'mivot-writer' of [email protected]:lmichel/pyvo.git into m…

d7c73e8

…ivot-writer

bsipocz reviewed Feb 15, 2025

View reviewed changes

andamian reviewed Feb 17, 2025

View reviewed changes

Adrian's review

01b6a5c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mivot writer #627

Mivot writer #627

lmichel commented Nov 30, 2024

codecov bot commented Dec 2, 2024 •

edited

Loading

bsipocz left a comment

bsipocz Dec 6, 2024

bsipocz Dec 6, 2024

lmichel commented Dec 15, 2024

bsipocz commented Dec 16, 2024

ManonMarchand commented Jan 6, 2025

andamian Jan 21, 2025

andamian Jan 21, 2025

lmichel Jan 23, 2025

bsipocz Feb 15, 2025

andamian Jan 21, 2025

andamian Jan 21, 2025

andamian Jan 22, 2025 •

edited

Loading

lmichel Jan 22, 2025

andamian Jan 22, 2025

lmichel Jan 23, 2025

bsipocz left a comment

bsipocz Feb 15, 2025

bsipocz Feb 15, 2025

andamian left a comment

andamian Jan 26, 2025

lmichel Feb 19, 2025

lmichel Feb 19, 2025

andamian Jan 27, 2025

andamian Jan 27, 2025

lmichel Feb 18, 2025 •

edited

Loading

lmichel Feb 19, 2025

andamian Jan 27, 2025

andamian Jan 27, 2025

andamian Jan 27, 2025

lmichel Feb 18, 2025

andamian Feb 18, 2025

lmichel Feb 19, 2025


		This module contains the new feature of annotations in VOTable.
		This module contains the new feature handling model annotations in VOTable.

	The example below shows how can be consumed a VOTable resulting from a cone-search query which data are mapped
	The example below shows how a VOTable result of a cone-search query can be parsed and data mapped

	It to be noted that ``mivot_viewer.xml_view`` is a shortcut
	It is to be noted that ``mivot_viewer.xml_view`` is a shortcut

		@@ -1,2 +1 @@
		# package entry point
		from .viewer.mivot_viewer import MivotViewer

Mivot writer #627

Are you sure you want to change the base?

Mivot writer #627

Conversation

lmichel commented Nov 30, 2024

codecov bot commented Dec 2, 2024 • edited Loading

Codecov Report

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

lmichel commented Dec 15, 2024

bsipocz commented Dec 16, 2024

ManonMarchand commented Jan 6, 2025

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

andamian Jan 22, 2025 • edited Loading

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

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

andamian 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

lmichel Feb 18, 2025 • edited Loading

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

codecov bot commented Dec 2, 2024 •

edited

Loading

andamian Jan 22, 2025 •

edited

Loading

lmichel Feb 18, 2025 •

edited

Loading