VOTable unit format by eerovaher · Pull Request #11032 · astropy/astropy

eerovaher · 2020-11-12T13:21:02Z

The unit format of a VOTable is version dependent. Astropy has the capability of parsing the new unit format, but nonetheless assumes all VOTables are using the old unit specification.

This pull request would make Astropy use the old 'cds' unit format up until VOTable version 1.3 and the new 'vounit' unit format starting from VOTable version 1.4.

Addresses #10791

The VOTable unit format should depend on its version (see issue astropy#10791). This commit adds a test that checks if the different VOTable versions have the correct unit format associated with them.

Because the unit format of a VOTable depends on its version the Field class must have access to this information, including in the test functions. This commit adds a function to VOTableFile that creates the appropriate Boolean flags. The same function can also be used in the VOTableFile parse() and to_xml() methods.

Using angstroms with the 'vounit' format raises a UnitsWarning that suggests angstroms should not be used. This warning causes test_votable_quantity_write() to fail even though the functionality of the underlying code is not compromised. Replacing angstroms with something else allows the test to pass.

mhvk

This looks all good to me - some nitpickly comments only. That said, as I'm really not familiar with vounit, I'd prefer to have @tomdonaldson sign off (in particular on choice of name/location of test/etc.

Also, for sure this needs an entry in CHANGES.rst! I guess one question is whether this is a bug fix and thus should be backported.

mhvk · 2020-11-12T21:30:38Z

                 config=None, pos=None, **extra):
        if config is None:
-            config = {}
+            if hasattr(votable, '_get_version_flags'):


Is it possible to pass in an old table?

I do not know if it is possible to pass in an old table, but without this change two tests fail because the correct unit format could not be determined.

OK, sounds like it is necessary, then, though I'll leave this to @tomdonaldson just to be sure.

I was wondering about this check as well. I think it is OK since It seems necessary to protect the unit tests which are exercising Field without a containing votable. Testing Field that way (or designing Field that way) may be questionable, but addressing those questions would be outside the scope of this PR.

mhvk · 2020-11-12T21:30:56Z

+
+
+def test_unit_format():
+    from astropy.io.votable.table import parse


Just put imports on top.

mhvk · 2020-11-12T21:31:30Z

        self.groups.append(group)
        group.parse(iterator, config)

+    def _get_version_flags(self):


I'd maybe call this _get_version_checks or _get_version_info

mhvk · 2020-11-12T21:32:56Z

    """
    t = QTable()
-    t['a'] = u.Quantity([1, 2, 4], unit='Angstrom')
+    t['a'] = u.Quantity([1, 2, 4], unit='m')


Does the new format not support Angstrom? My tendency would then be to use 'nm' to at least keep it a prefixed unit.

Angstroms are supported but discouraged, so using them raises a UnitsWarning which seems to cause this test to fail. Running pytest -P table -p no:warnings does succeed.

OK, let's use 'nm' instead, then.

eerovaher · 2020-11-14T17:20:59Z

This is the current warning about an invalid unit string in a VOTable:

astropy/astropy/io/votable/exceptions.py

Line 1040 in 790332a

class W50(VOTableSpecWarning):

It is correct for the older VOTable versions, but for newer ones the warning should refer to the VOUnit specification instead. Should I add W56 with the appropriate reference for use with the newer VOTables?

mhvk · 2020-11-15T02:27:12Z

@eerovaher - yes, seems good to update the warning too! (though again, I know very little about VOTable - I was asked because of units!)

tomdonaldson · 2020-11-15T19:28:28Z

This is the current warning about an invalid unit string in a VOTable:

astropy/astropy/io/votable/exceptions.py

Line 1040 in 790332a

class W50(VOTableSpecWarning):

It is correct for the older VOTable versions, but for newer ones the warning should refer to the VOUnit specification instead. Should I add W56 with the appropriate reference for use with the newer VOTables?

Yes, I think it would be great to add that new unit information to the warning(s). Would it be easier to add the info to the existing warning so that the one warning covers both cases? I don't have a strong opinion one way or the other.

I did notice some confusion in the related issue #10791 regarding the most up-to-date VOUnits specification. (It looks like there is a mixup in some IVOA document URL redirects.) The best URL for the latest VOUnits specification is: https://www.ivoa.net/documents/VOUnits

tomdonaldson

Thanks very much @eerovaher for this PR, and @mhvk for the review.

I agree with the analysis regarding the versioning of units with respect to VOTable version, i.e. that the change in official VOTable unit syntax from cds to vounit occurs with VOTable 1.4.

I also agree that these changes fix the behavior correctly, so I will approve this with an update to the warning(s) as discussed.

Since this behavior should have been included with the initial support for VOTable 1.4, this should probably be treated as a bug fix and backported.

The unit format of a VOTable is version-dependent. The docstring of the VOTable invalid unit string warning (W50) should therefore contain references to both the old and the new unit format specification.

eerovaher · 2020-11-16T13:41:11Z

I have updated W50 and I can update CHANGES.rst when the milestone for this pull request is reassigned.

tomdonaldson · 2020-11-16T16:24:44Z

@eerovaher thanks, this looks great. Although this is fixing a bug from 4.0, I'm second guessing whether or not it warrants a backport. Would a 4.3 release meet your needs?

eerovaher · 2020-11-16T16:36:25Z

I don't have a preference for which release this should be included in.

tomdonaldson · 2020-11-16T16:44:16Z

I don't have a preference for which release this should be included in.

OK, sounds good. I'll leave the milestone at 4.3 then.

tomdonaldson

This all looks good. Thanks again for these changes!

bsipocz · 2021-01-27T03:34:42Z

Hmm, my bad, my issue is not with this PR, but with another one. I'm sorry about the noise.

eerovaher added 4 commits November 11, 2020 19:10

Add VOTable unit format recognition test

121d0bd

The VOTable unit format should depend on its version (see issue astropy#10791). This commit adds a test that checks if the different VOTable versions have the correct unit format associated with them.

Make VOTable unit format version-dependent astropy#10791

21d3ef3

github-actions Bot added io.votable table labels Nov 12, 2020

eerovaher mentioned this pull request Nov 12, 2020

VOTable unit parsing #10791

Closed

pllim requested review from mhvk and tomdonaldson November 12, 2020 16:06

pllim added this to the v4.3 milestone Nov 12, 2020

mhvk reviewed Nov 12, 2020

View reviewed changes

eerovaher added 2 commits November 13, 2020 14:08

Address feedback given in pull request astropy#11032

6c909fa

Update documentation (pull request astropy#11032)

4a70d39

tomdonaldson requested changes Nov 15, 2020

View reviewed changes

Update VOTable invalid unit string warning

f63240f

The unit format of a VOTable is version-dependent. The docstring of the VOTable invalid unit string warning (W50) should therefore contain references to both the old and the new unit format specification.

tomdonaldson approved these changes Nov 16, 2020

View reviewed changes

tomdonaldson merged commit c76ffaa into astropy:master Nov 16, 2020

bsipocz mentioned this pull request Jan 27, 2021

MNT: workaround astropy unit parsing changes astropy/astroquery#1961

Merged

namurphy pushed a commit to namurphy/astropy that referenced this pull request Feb 21, 2021

Address feedback given in pull request astropy#11032

7260cc2

namurphy pushed a commit to namurphy/astropy that referenced this pull request Feb 21, 2021

Update documentation (pull request astropy#11032)

df62103

namurphy pushed a commit to namurphy/astropy that referenced this pull request Feb 21, 2021

Address feedback given in pull request astropy#11032

f8fced1

namurphy pushed a commit to namurphy/astropy that referenced this pull request Feb 21, 2021

Update documentation (pull request astropy#11032)

4dee433

eerovaher deleted the votable-units branch April 26, 2021 14:01

ManonMarchand mentioned this pull request Dec 4, 2024

Cannot write votable with units equivalent to Jy/beam #17497

Closed

pllim mentioned this pull request Dec 19, 2024

BUG: Fix VOTable unit format on write so it does not hardcode to CDS format always #17570

Merged

1 task



		def test_unit_format():
		from astropy.io.votable.table import parse

Uh oh!

Uh oh!

Conversation

eerovaher commented Nov 12, 2020

Uh oh!

mhvk 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!

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!

Choose a reason for hiding this comment

Uh oh!

eerovaher commented Nov 14, 2020

Uh oh!

mhvk commented Nov 15, 2020

Uh oh!

tomdonaldson commented Nov 15, 2020

Uh oh!

tomdonaldson left a comment

Choose a reason for hiding this comment

Uh oh!

eerovaher commented Nov 16, 2020

Uh oh!

tomdonaldson commented Nov 16, 2020

Uh oh!

eerovaher commented Nov 16, 2020

Uh oh!

tomdonaldson commented Nov 16, 2020

Uh oh!

tomdonaldson left a comment

Choose a reason for hiding this comment

Uh oh!

bsipocz commented Jan 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants