Feature MIVOT (Model Instance in VOTable) #15390
Conversation
|
Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.
|
|
pre-commit.ci autofix |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to Astropy 👋 and congratulations on your first pull request! 🎉
A project member will respond to you as soon as possible; in the meantime, please have a look over the Checklist for Contributed Code and make sure you've addressed as many of the questions there as possible.
If you feel that this pull request has not been responded to in a timely manner, please send a message directly to the development mailing list. If the issue is urgent or sensitive in nature (e.g., a security vulnerability) please send an e-mail directly to the private e-mail feedback@astropy.org.
|
Re: #15390 (comment) Not exactly. The milestone means this feature is currently slated for v6.0 release. We move the milestone if this PR does not get merged before feature freeze (also see https://github.com/astropy/astropy/wiki/Release-Calendar). |
There was a problem hiding this comment.
Thanks! It is my understanding that PyVO is a stakeholder and its developers would review this in depth later. I am just leaving a general review.
Unless you are willing to let maintainer use "squash and merge" button for this PR, all the merge commits in your commit history here need to go (a rebase should do it in most cases).
Probably needs a What's New entry too.
If the API or behavior is expected to change after this code is released, please also add a warning box in the user documentation saying that the API or behavior is unstable and please be specific.
For the documentation that gets rendered to be read by users, please be consistent in capitalization (mivot vs MIVOT vs Mivot, VOtable vs VOTable, and so on). If different capitalization signifies different things (e.g., astropy vs Astropy), that is fine but you might have to explain.
When this PR is ready for final review, might want to add the "Extra CI" label too, just in case.
| @@ -175,3 +177,333 @@ def test_votable_tag(): | |||
| assert 'xmlns="http://www.ivoa.net/xml/VOTable/v1.3"' in xml | |||
| assert 'xsi:schemaLocation="http://www.ivoa.net/xml/VOTable/v1.3 ' in xml | |||
| assert 'http://www.ivoa.net/xml/VOTable/VOTable-1.4.xsd"' in xml | |||
|
|
|||
|
|
|||
| def squash_xml(data): | |||
There was a problem hiding this comment.
Hmm, no better way to do this? If not, is this function better in astropy/utils/xml? Or is this function not useful beyond this test module?
There was a problem hiding this comment.
We think this function won't be useful beyond the test module.
We tried to do it in other way: using etree.XMLParser() or using re.sub() with regexep, in the end this function was more convenient and simpler.
| # a mivot block must be with "type=meta" | ||
| mivot_resource.mivot_block = mivot_block | ||
| raise AssertionError() | ||
| except E26: |
| assert True | ||
|
|
||
|
|
||
| def setup_function(test_mivot_order): |
There was a problem hiding this comment.
I don't understand why setup/teardown is needed here. Why not use tmp_path for your test?
There was a problem hiding this comment.
I have always wanted to kill off the example gallery. I see it as a failed experiment that had since been superseded by Learn Astropy efforts outside the core library. I'd rather we not add more stuff to it now. Also see #7242 (comment)
Can this example be integrated into docs/io/votable somewhere instead?
|
Also, please note that v6.0 feature freeze is planned for Oct 2023 in about a month (https://github.com/astropy/astropy/wiki/Release-Calendar). So your collaborators and stakeholders should start reviewing this sooner than later, or it might not make the cut. |
|
p.s. For future reference, you could accept suggested changes in batch mode. |
Thank you for the general review, changes will be coming soon. Regarding the messy commit history, we would like a squash and merge if possible. |
|
|
||
| data_path = os.path.dirname(os.path.realpath(__file__)) | ||
| vpath = os.path.join(data_path, "data/test.order.xml") | ||
| vpath_out = os.path.join(data_path, tmp_path / "test.order.out.xml") |
There was a problem hiding this comment.
You want to limit writing out test file only to the temporary test dir.
| vpath_out = os.path.join(data_path, tmp_path / "test.order.out.xml") | |
| vpath_out = str(tmp_path / "test.order.out.xml") |
There was a problem hiding this comment.
Thank you for the precision, changes have been made
| data_path = os.path.dirname(os.path.realpath(__file__)) | ||
| vpath = os.path.join(data_path, "data/test.order.xml") |
There was a problem hiding this comment.
I think you can just use our built-in get_pkg_data_filename() from astropy.utils.data.
| data_path = os.path.dirname(os.path.realpath(__file__)) | |
| vpath = os.path.join(data_path, "data/test.order.xml") | |
| vpath = get_pkg_data_filename("data/test.order.xml") |
|
You might not want to enable doctest on your What's New entry since it is trying to read a non-existent file. If you do want to use a test file you added, then you can do something like this instead: >>> from astropy.utils.data import get_pkg_data_filename
>>> filename = get_pkg_data_filename("data/votable.xml", package="astropy.io.votable.tests") # If this file is in io/votable/tests/data, for example
# Then you can run the rest of the doctest with this filename |
|
Hm... while I agree that astropy-core probably shouldn't know too much about DMs as such, I'd say that the API implemented here is too poor to make much sense, for all I can see not even as a basis for later, more refined APIs: On reading, what do you expect me to do with your MivotBlock? On writing, how do I come up with it? For an alternative DM annotation, I once have implemented a somewhat more concrete API in https://github.com/msdemlei/astropy, where you could say things like and then get back objects that would support attribute access for the various class members somewhat like this: I give you that because MIVOT is a lot more complex than the annotation scheme proposed in msdemlei/astropy, perhaps a less straightforward API may be called for – but then, having something like get_annotations even just for a subset of MIVOT would, I think, help people doing actual stuff once we have DM annotations out in the wild a lot more than the fairly opaque MivotBlock. |
Markus, You are right our API is very poor. Actually it does nothing except reading and writing XML strings and nothing in the ASTROPY code can work with that, this is true. These IO operations are implemented in AStropy just because the VOTable parser is here. |
|
Anyone interested in this PR has any issue with changes to |
After checking the lines, this PR shouldn't conflict or impact MIVOT PR. |
|
p.s. Once VO reviewers approved, I would still go through the code one more time to make sure it adheres to Astropy standards. So please do not merge yet. But let me know if I can merge when I am happy with it. Thanks! |
Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
|
Something went wrong with the commit history. I thought @bsipocz squashed the stuff into 14 commits last Friday. So with 2 new commits, it should have been 16. But I see 56 commits now. And the CI failure does not make sense. So something is wrong. |
|
I suspect the old branch got force-pushed back rather than the rebased version pulled and updated. I still have the 14-commit version around locally and can push it back. As I recall that one was passing CI and was waiting for your review., but was otherwise ready to go. |
|
Ahh, looking at the branch structure, the second to last commit is still correct, the problem is with the last one only, as I suspect that was done locally on the outdated branch while second to last was done directly on github. |
Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
|
devdeps failure is unrelated (numpy-dev again), so merging. Thanks for your patience and contribution, all! |
|
Great, tanks to all of the reviewers. |
Reading and Writing VO Model Annotations
Introduction
Model Instances in VOTables (MIVOT) defines a syntax for mapping VOTable data to any model serialized in VO-DML (Virtual Observatory Data Modeling Language).
This annotation schema acts as a bridge between data and models. It associates both column/parameter metadata and VOTable data with data model elements (such as classes, attributes, types, etc.). It also complements VOTable data or metadata that may be missing from the table, e.g., coordinate system descriptions or curation tracing.
The data model elements are organized in an independent annotation block complying with the MIVOT XML schema, which is added as an extra resource above the TABLE element. The MIVOT syntax allows a data structure to be described as a hierarchy of classes. It can also represent relationships and compositions between them. Furthermore, it can construct data model objects by aggregating instances from different tables of the VOTable.
Astropy Implementation
The purpose of Astropy is not to process VO annotations. Instead, it allows related packages to get and set Mivot blocks from/into VOTables. Therefore, in this implementation, MIVOT annotations are both imported and exported as strings. The current implementation prevents client code from injecting into VOTables strings that are not Mivot serializations.
MivotBlockClass:MivotBlockclass intree.py.MivotBlockinstance can only be carried by a resource with "type=meta."MivotBlockobjects are instantiated by the Resource parser.MivotBlockclass has its logic that operates both parsing and I/O functionalities.Affected Modules:
tree.py: Creation of theMivotBlock, addition of aMivotBlockinstance to theResourceclass as a property, and inclusion of a call to the MIVOT parser when theResourceparser encounters a VODML tag.writer.py: The functionstring_element()has been added to indent theMivotBlockXML before pushing it in the writer.test_7_votable_pointer.py: Some usage examples are described inexamples/mivotblock.exceptions.py: Addition of warning (ModelMappingSpecWarning) and error (E26) classes related to MIVOT.Description
This pull request is for a new feature that allows the reading and the writing of model annotations in VOTable
Fixes #12153