Make QTable unit parsing more robust by eerovaher · Pull Request #10786 · astropy/astropy

eerovaher · 2020-09-30T17:44:51Z

So far QTable has been much more stringent about the unit format than
units.Unit. For example, units such as km.s**-1 are not recognized by
QTable even though Unit is able to parse them just fine. As a result
QTable does not convert columns with such units into Quantities even
though it should. This update fixes the issue by making use of the unit
parsing capabilities that Unit already has.

So far QTable has been much more stringent about the unit format than units.Unit. For example, units such as km.s**-1 are not recognized by QTable even though Unit is able to parse them just fine. As a result QTable does not convert columns with such units into Quantities even though it should. This update fixes the issue by making use of the unit parsing capabilities that Unit already has.

pllim · 2020-09-30T19:13:37Z

Thanks! Would need to add a test to address the missing coverage and also add a change log.

mhvk

In principle, this looks good (though I managed two comments...) but most important is to add a test case that currently fails and that is fixed by this PR.

I ask in particular since I am surprised this is really needed: q_cls should either be Quantity or a subclass, and those already pass the unit through Unit. So, if it fails, it may also be that there is a bug in units instead of here...

mhvk · 2020-09-30T19:22:04Z

        return has_info_class(col, MixinInfo)

    def _convert_col_for_table(self, col):
+        from astropy.units import Unit


You can just add Unit to the line from astropy.units import Quantity, QuantityInfo at the top.

Done in 2f0ab7b

mhvk · 2020-09-30T19:23:34Z

            # We need to turn the column into a quantity, or a subclass
            # identified in the unit (such as u.mag()).
            q_cls = getattr(col.unit, '_quantity_class', Quantity)
-            qcol = q_cls(col.data, col.unit, copy=False)


I would change this to Unit(col.unit) and omit the try/except.

Unit(col.unit) does not fix the problem, the intermediate conversion to a string is necessary.

If Unit() cannot parse the unit then it will throw a ValueError. The try/except statement ensures that if this happens the code will revert to the old behavior. Omitting try/except could make some files unreadable.

pep8speaks · 2020-10-01T14:51:21Z

Hello @eerovaher 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

There are no PEP8 style issues with this pull request - thanks! 🎉

Comment last updated at 2020-10-01 14:56:30 UTC

eerovaher · 2020-10-01T15:31:50Z

It looks like I had not quite understood the nature of the problem. The columns do get converted to Quantities, but in the cases relevant for this pull request the unit of the resulting Quantity is an UnrecognizedUnit instance, which makes the Quantity instance rather useless.

This pull request does ensure the expected behavior both when existing Tables are converted to QTables and when the QTable.read() method is invoked. I believe that these two are by far the most frequent cases where the issue might pop up, so in practice the solution could be considered satisfactory. If you agree, then I will write up the changelog entry. But the actual problem might be in unit parsing done in the Quantity constructor instead.

mhvk · 2020-10-01T16:47:52Z

@eerovaher - I can see how your solution works, but I think it papers over the real bug, that by the time _convert_col_for_table is called, the input col argument already has an UnrecognizedUnit as its unit. Indeed, just replacing QTable.read in your test by Table.read, one gets the same problem. This is also why you need to go through Unit(str(unit)) - since col.unit is already a UnitBase instance, no attempt is made to parse it again.

So, the problem would seem to be higher up. One can trace it back with:

from astropy.utils.data import get_pkg_data_filename
from astropy.table import QTable

filename = get_pkg_data_filename('data/nonstandard_units.xml', 'astropy.io.votable.tests')
QTable.read(filename, verify='exception')
---------------------------------------------------------------------------
W50                                       Traceback (most recent call last)
<ipython-input-1-c39669ef4942> in <module>
      3 
      4 filename = get_pkg_data_filename('data/nonstandard_units.xml', 'astropy.io.votable.tests')
----> 5 QTable.read(filename, verify='exception')

/usr/lib/python3/dist-packages/astropy/table/connect.py in __call__(self, *args, **kwargs)
     50     def __call__(self, *args, **kwargs):
     51         cls = self._cls
---> 52         out = registry.read(cls, *args, **kwargs)
     53 
     54         # For some readers (e.g., ascii.ecsv), the returned `out` class is not

/usr/lib/python3/dist-packages/astropy/io/registry.py in read(cls, format, *args, **kwargs)
    521 
    522         reader = get_reader(format, cls)
--> 523         data = reader(*args, **kwargs)
    524 
    525         if not isinstance(data, cls):

/usr/lib/python3/dist-packages/astropy/io/votable/connect.py in read_table_votable(input, table_id, use_names_over_ids, verify)                                                                               
     79     """
     80     if not isinstance(input, (VOTableFile, VOTable)):
---> 81         input = parse(input, table_id=table_id, verify=verify)
     82 
     83     # Parse all table objects

/usr/lib/python3/dist-packages/astropy/utils/decorators.py in wrapper(*args, **kwargs)
    533                     warnings.warn(message, warning_type, stacklevel=2)
    534 
--> 535             return function(*args, **kwargs)
    536 
    537         return wrapper

/usr/lib/python3/dist-packages/astropy/io/votable/table.py in parse(source, columns, invalid, verify, chunk_size, table_number, table_id, filename, unit_format, datatype_mapping, _debug_python_based_parser)
    164             source,
    165             _debug_python_based_parser=_debug_python_based_parser) as iterator:
--> 166         return tree.VOTableFile(
    167             config=config, pos=(1, 1)).parse(iterator, config)
    168 

/usr/lib/python3/dist-packages/astropy/io/votable/tree.py in parse(self, iterator, config)
   3565         for start, tag, data, pos in iterator:
   3566             if start:
-> 3567                 tag_mapping.get(tag, self._add_unknown_tag)(
   3568                     iterator, tag, data, config, pos)
   3569             elif tag == 'DESCRIPTION':

/usr/lib/python3/dist-packages/astropy/io/votable/tree.py in _add_resource(self, iterator, tag, data, config, pos)                                                                                            
   3476         resource = Resource(config=config, pos=pos, **data)
   3477         self.resources.append(resource)
-> 3478         resource.parse(self, iterator, config)
   3479 
   3480     def _add_coosys(self, iterator, tag, data, config, pos):

/usr/lib/python3/dist-packages/astropy/io/votable/tree.py in parse(self, votable, iterator, config)
   3277         for start, tag, data, pos in iterator:
   3278             if start:
-> 3279                 tag_mapping.get(tag, self._add_unknown_tag)(
   3280                     iterator, tag, data, config, pos)
   3281             elif tag == 'DESCRIPTION':

/usr/lib/python3/dist-packages/astropy/io/votable/tree.py in _add_table(self, iterator, tag, data, config, pos)                                                                                               
   3223         table = Table(self._votable, config=config, pos=pos, **data)
   3224         self.tables.append(table)
-> 3225         table.parse(iterator, config)
   3226 
   3227     def _add_info(self, iterator, tag, data, config, pos):

/usr/lib/python3/dist-packages/astropy/io/votable/tree.py in parse(self, iterator, config)
   2452                         break
   2453 
-> 2454                     tag_mapping.get(tag, self._add_unknown_tag)(
   2455                         iterator, tag, data, config, pos)
   2456                 else:

/usr/lib/python3/dist-packages/astropy/io/votable/tree.py in _add_field(self, iterator, tag, data, config, pos)                                                                                               
   2368 
   2369     def _add_field(self, iterator, tag, data, config, pos):
-> 2370         field = Field(self._votable, config=config, pos=pos, **data)
   2371         self.fields.append(field)
   2372         field.parse(iterator, config)

/usr/lib/python3/dist-packages/astropy/io/votable/tree.py in __init__(self, votable, ID, name, datatype, arraysize, ucd, unit, width, precision, utype, ref, type, id, xtype, config, pos, **extra)
   1218         self.arraysize = arraysize
   1219         self.ucd = ucd
-> 1220         self.unit = unit
   1221         self.width = width
   1222         self.precision = precision

/usr/lib/python3/dist-packages/astropy/io/votable/tree.py in unit(self, unit)
   1386             unit, format=default_format, parse_strict='silent')
   1387         if isinstance(unit_obj, u.UnrecognizedUnit):
-> 1388             warn_or_raise(W50, W50, (unit,),
   1389                           self._config, self._pos)
   1390 

/usr/lib/python3/dist-packages/astropy/io/votable/exceptions.py in warn_or_raise(warning_class, exception_class, args, config, pos, stacklevel)
     88         if exception_class is None:
     89             exception_class = warning_class
---> 90         vo_raise(exception_class, args, config, pos)
     91     elif config_value == 'warn':
     92         vo_warn(warning_class, args, config, pos, stacklevel=stacklevel+1)

/usr/lib/python3/dist-packages/astropy/io/votable/exceptions.py in vo_raise(exception_class, args, config, pos)
     99     if config is None:
    100         config = {}
--> 101     raise exception_class(args, config, pos)
    102 
    103 

W50: None:9:0: W50: Invalid unit string 'erg cm**(-2) s**(-1) angstrom**(-1)'

Tracing it back, the file is read by io.votable, inside tree.py, where (I think) the following is done:

u.Unit('erg cm**(-2) s**(-1) angstrom**(-1)', format='cds', parse_strict='silent')
# UnrecognizedUnit(erg cm**(-2) s**(-1) angstrom**(-1))

mhvk · 2020-10-01T16:56:04Z

Continuing from the above, I think this is not an issue with QTable, but rather a question of how strict one wants to be in checking whether units are consistent with the VO standard. I think in principle votable can handle passing a different unit interpreter, which would make it less strict, but it is not clear this is exposed via the QTable.read interface (which goes to io.votable.connect.read_table_votable, I think). As I'm not very familiar with io.vounit, I think I better cc @tboch, @tomdonaldson, and @pllim.

p.s. It may be better to start a new issue, since I think it is clear that the solution proposed here attacks the problem at the wrong level - though we should definitely keep the test!

mhvk · 2020-10-01T16:58:11Z

@tboch, @tomdonaldson, @pllim - just to be clear, I think the case that this PR tries to solve is how to read a VOtable file with non-compliant but otherwise quite obvious to understand units in such a way that the units are regular ones, not UnrecognizedUnit.

pllim · 2020-10-01T17:23:33Z

I would say anything that isn't supported at http://www.ivoa.net/documents/VOTable/20191021/REC-VOTable-1.4-20191021.html#ToC29 and http://www.ivoa.net/documents/VOUnits/ (PDF) should not be supported here, but I'll have to defer to @tomdonaldson and @tboch .

Disclaimer: Haven't had time to see if the offending unit string is really supported or not.

mhvk · 2020-10-01T17:35:45Z

I think the unit is not supported, otherwise the file wouldn't be called nonstandard_units.xml 😄. But one cannot always control the contents of files one has to read, so the question would seem to be whether there should be a (non-default) option to just parse the units with, say, the generic unit parser and get out a useful table. Of course, when writing we should not do that: generous on input, strict on output and all that.

My sense is that inside io.votable this is already possible - what is missing is the ability to use that capability from within Table.read(). (which suggests also pinging @taldcroft)

eerovaher · 2020-10-01T18:38:25Z

The _get_default_unit_format() function in astropy/io/votable/tree.py might be in need of updating. If I simply change it to return 'vounit'instead of 'cds' I can at least properly read in Gaia archive queries as QTables, though this also seems to cause a few test to fail and the proper solution needs to be more clever.

Should I revert the changes made to the astropy/table/table.py file here or should I make a new pull request for the proper fix?

mhvk · 2020-10-01T20:21:49Z

@eerovaher - that indeed seems the right place! My sense would be to make a new PR - though I'd definitely keep your added test. We for sure want to be able to read GAIA VO tables!

pllim · 2020-10-02T13:20:12Z

Re: #10786 (comment)

Note for devs: 'cds' has been the default from the beginning (even though this particular function was added in #620). This change, if we choose to proceed, cannot go into any bug fix release and has to be tested by different stakeholders before merge.

eerovaher · 2020-10-02T15:46:46Z

While the problem this pull request was meant to address is real, the solution proposed here is a workaround, not a bug fix. I have opened the issue #10791 for the actual underlying problem. Furthermore, the test added in this pull request is in conflict with this test, so it looks like this pull request can be closed without merging anything.

taldcroft · 2020-10-03T10:20:17Z

Closing per comment above from @eerovaher.

astropy-bot Bot added the table label Sep 30, 2020

pllim added this to the v4.2 milestone Sep 30, 2020

pllim added the Enhancement label Sep 30, 2020

pllim requested review from mhvk and taldcroft September 30, 2020 19:13

mhvk requested changes Sep 30, 2020

View reviewed changes

eerovaher added 2 commits October 1, 2020 16:44

Add test for QTable unit parsing fix astropy#10786

9363e7e

Move an import to the top of the file

2f0ab7b

Add blank lines for PEP 8 compliance

386e501

eerovaher mentioned this pull request Oct 2, 2020

VOTable unit parsing #10791

Closed

pllim added Close? Tell stale bot that this issue/PR is stale io.votable labels Oct 2, 2020

taldcroft closed this Oct 3, 2020

eerovaher deleted the qtable-parse-unit branch April 26, 2021 13:59

Conversation

eerovaher commented Sep 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim commented Sep 30, 2020

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

mhvk Sep 30, 2020

Choose a reason for hiding this comment

Uh oh!

eerovaher Oct 1, 2020

Choose a reason for hiding this comment

Uh oh!

mhvk Sep 30, 2020

Choose a reason for hiding this comment

Uh oh!

eerovaher Oct 1, 2020

Choose a reason for hiding this comment

Uh oh!

pep8speaks commented Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2020-10-01 14:56:30 UTC

Uh oh!

eerovaher commented Oct 1, 2020

Uh oh!

mhvk commented Oct 1, 2020

Uh oh!

mhvk commented Oct 1, 2020

Uh oh!

mhvk commented Oct 1, 2020

Uh oh!

pllim commented Oct 1, 2020

Uh oh!

mhvk commented Oct 1, 2020

Uh oh!

eerovaher commented Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented Oct 1, 2020

Uh oh!

pllim commented Oct 2, 2020

Uh oh!

eerovaher commented Oct 2, 2020

Uh oh!

taldcroft commented Oct 3, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

eerovaher commented Sep 30, 2020 •

edited

Loading

pep8speaks commented Oct 1, 2020 •

edited

Loading

eerovaher commented Oct 1, 2020 •

edited

Loading