Relax the VOTable requirements when parsing units - #620

How can we no what to do with the unit, if it doesn't conform to the standard? There are two many adhoc ways that people spell units to possibly know. I'd rather have an option to the parser that allows the user to "select" a given unit format if they know that cds is not the one in the file.

I might be overlooking something, but as far as I can see, the call
unit = u.Unit(unit, format='cds', parse_strict='silent')

might be be replaced by
unit = u.Unit(unit, parse_strict='silent')

when the votable parser is called with pedantic=False.

The only thing that changes is that calls like unit = u.Unit('Angstrom', parse_strict='silent') will not return an UnrecognizedUnit. I am not asking to the framework to be able to parse any string, only to honor the pedantic=False clause.

Also, the VOUnits standard, which includes more symbols and different grammars than the current 'cds' one alone, is about to be approved by the IVOA.

The FITS unit format (which is the default in astropy) and the CDS unit format also have different syntaxes. So are you suggesting we use CDS syntax but support more symbols? It's possible, given the direction of VO, that falling back to the VOUnit format would be a better choice, though. I agree that it would be nice if pedantic=False did something, that what I'm getting at is that there is no one obvious choice of format to fall back on, so the user would need to provide it. Or we could experiment with trying CDS, VOUnit and FITS in some order until one succeeds, but I worry that trying to be too smart may just lead to confusion for the end user.

We already have the VOUnit format implemented so we can hit the ground running once that format is approved -- but likely that will only be switched on for VOTable v1.3 files and later (because obviously the unit format change won't be retroactive).

There are two concerns: one is to comply with the standards, the other is to make the library useful for users. I'll tweak my suggestion: it would be nice to provide the user with the ability to use different unit parsers, especially (if not only) with pedantic=False. I had to change the code for being able to parse 'Angstrom', which is quite a common unit string out there in the wild. Being able to specify the format would be much better. Generalizing, it would be probably a good idea to pass a configuration object (a dictionary?) to the votable.parse() method, so that in the future new options might be passed to the parser without having to tweak its interface over and over again, but that's just an idea.

Thanks.

BTW, 'Angstrom' (and more generally the OGIP notation, e.g. 'erg cm**(-2) s**(-1) angstrom**(-1)') is used in some of the IVOA RECs, including specific VOTable examples. See for example
http://ivoa.net/Documents/SpectrumDM/20111120/REC-SpectrumDM-1.1-20111120.pdf

Oh, dear. Definitely a case of one hand not knowing what the other is doing ;) Expect a PR to handle this shortly.

@OmarLaurino : Can you let me know if this PR addresses your need?

The code looks great, I'll need to test it later this week, though. Thanks!

@OmarLaurino: Have you had a chance to look at this?

This doesn't seem to work:

t=votable.parse('/Users/olaurino/FilteredNED+WISE.vot', pedantic=False, unit_format='vounit')
t0=t.get_table_by_index(0)
t0.get_field_by_id_or_name('ID_1').unit
Out[17]: UnrecognizedUnit(erg s**(-1) cm**(-2) Hz**(-1))

Where this is the xml for ID_1:

<FIELD ID="ID_1" datatype="double" unit="erg s**(-1) cm**(-2) Hz**(-1)" utype="spec:Spectrum.Data.FluxAxis.Value"/>

However:

u.Unit("erg s**(-1) cm**(-2) Hz**(-1)", format='vounit')
Out[18]: Unit("erg / (cm2 Hz s)")

Am I doing something wrong?

Strange. It works for me. Are you sure you're running the latest version of this branch? If that's the case, can you send me the file so I can have a look?

Did you receive the file?

Yes -- I have the file. I'm looking into it now.

Hmm... I can't reproduce. In your e-mail, you said you were testing 74ce8aa, but the most recent commit on this branch is af4bef4 -- that's what you should be testing with. Can you confirm whether that commit works?

In [1]: from astropy.io import votable

In [2]: votable.parse("FilteredNED+WISE.vot", unit_format='vounit')
WARNING: W50: FilteredNED+WISE.vot:24:10: W50: Invalid unit string 'Angstrom' [astropy.io.votable.exceptions]
WARNING: W50: FilteredNED+WISE.vot:27:10: W50: Invalid unit string 'erg s**(-1) cm**(-2) Hz**(-1)' [astropy.io.votable.exceptions]
WARNING: W15: FilteredNED+WISE.vot:47:6: W15: FIELD element missing required 'name' attribute [astropy.io.votable.exceptions]
WARNING: W50: FilteredNED+WISE.vot:47:6: W50: Invalid unit string 'Angstrom' [astropy.io.votable.exceptions]
WARNING: W15: FilteredNED+WISE.vot:48:6: W15: FIELD element missing required 'name' attribute [astropy.io.votable.exceptions]
WARNING: W50: FilteredNED+WISE.vot:48:6: W50: Invalid unit string 'erg s**(-1) cm**(-2) Hz**(-1)' [astropy.io.votable.exceptions]
WARNING: W15: FilteredNED+WISE.vot:49:6: W15: FIELD element missing required 'name' attribute [astropy.io.votable.exceptions]
WARNING: W06: FilteredNED+WISE.vot:49:6: W06: Invalid UCD 'stat.error;phot.flux.density;em.*': UCD has invalid character '*' in 'stat.error;phot.flux.density;em.*' [astropy.io.votable.exceptions]
WARNING: W50: FilteredNED+WISE.vot:75:10: W50: Invalid unit string 'Angstrom' [astropy.io.votable.exceptions]
WARNING: W50: FilteredNED+WISE.vot:78:10: W50: Invalid unit string 'erg s**(-1) cm**(-2) Hz**(-1)' [astropy.io.votable.exceptions]
WARNING: W15: FilteredNED+WISE.vot:98:6: W15: FIELD element missing required 'name' attribute [astropy.io.votable.exceptions]
WARNING: W50: FilteredNED+WISE.vot:98:6: W50: Invalid unit string 'Angstrom' [astropy.io.votable.exceptions]
WARNING: W15: FilteredNED+WISE.vot:99:6: W15: FIELD element missing required 'name' attribute [astropy.io.votable.exceptions]
WARNING: W50: FilteredNED+WISE.vot:99:6: W50: Invalid unit string 'erg s**(-1) cm**(-2) Hz**(-1)' [astropy.io.votable.exceptions]
WARNING: W15: FilteredNED+WISE.vot:100:6: W15: FIELD element missing required 'name' attribute [astropy.io.votable.exceptions]
WARNING: W06: FilteredNED+WISE.vot:100:6: W06: Invalid UCD 'stat.error;phot.flux.density;em.*': UCD has invalid character '*' in 'stat.error;phot.flux.density;em.*' [astropy.io.votable.exceptions]
WARNING: W50: FilteredNED+WISE.vot:126:10: W50: Invalid unit string 'Angstrom' [astropy.io.votable.exceptions]
WARNING: W50: FilteredNED+WISE.vot:129:10: W50: Invalid unit string 'erg s**(-1) cm**(-2) Hz**(-1)' (suppressing further warnings of this type...) [astropy.io.votable.exceptions]
WARNING: W15: FilteredNED+WISE.vot:149:6: W15: FIELD element missing required 'name' attribute [astropy.io.votable.exceptions]
WARNING: W15: FilteredNED+WISE.vot:150:6: W15: FIELD element missing required 'name' attribute [astropy.io.votable.exceptions]
WARNING: W15: FilteredNED+WISE.vot:151:6: W15: FIELD element missing required 'name' attribute [astropy.io.votable.exceptions]
WARNING: W06: FilteredNED+WISE.vot:151:6: W06: Invalid UCD 'stat.error;phot.flux.density;em.*': UCD has invalid character '*' in 'stat.error;phot.flux.density;em.*' [astropy.io.votable.exceptions]
WARNING: W15: FilteredNED+WISE.vot:1020:6: W15: FIELD element missing required 'name' attribute (suppressing further warnings of this type...) [astropy.io.votable.exceptions]
WARNING: W06: FilteredNED+WISE.vot:1022:6: W06: Invalid UCD 'stat.error;phot.flux.density;em.*': UCD has invalid character '*' in 'stat.error;phot.flux.density;em.*' [astropy.io.votable.exceptions]
Out[2]: <astropy.io.votable.tree.VOTableFile at 0x389f710>

In [3]: x = _

In [4]: t0 = x.get_ta
x.get_table_by_id     x.get_table_by_index  

In [4]: t0 = x.get_table_by_index(0)

In [5]: t0.get_fiel
t0.get_field_by_id          t0.get_field_by_id_or_name  

In [5]: t0.get_field_by_id_or_name('ID_1').unit
Out[5]: Unit("erg / (cm2 Hz s)")

It works, sorry for the mistake.

Thank you!