Parse CDS units with Unit 'cds' format. by kyleaoman · Pull Request #7348 · astropy/astropy

kyleaoman · 2018-04-04T15:13:28Z

The astropy.units.Unit class has an option 'format' in the constructor which accepts a parameter 'cds' so that units formatted conforming to the CDS standard can be parsed correctly. This change sets the column units of the read table using this feature. Previously, the columns of the read table would have units of type UnrecognizedUnit (basically a glorified string) in many cases - I stumbled on this when trying to read a table with a column in GMsun (giga-solar-masses). I believe this should be fully backwards-compatible from a user point of view.

The following snippet will fail before this change, and succeed after:

from astropy.io import ascii
import astropy.units as u

reader = ascii.get_reader(ascii.Cds, readme='ftp://cdsarc.u-strasbg.fr/pub/cats/J/AJ/152/157/ReadMe')
SPARC = reader.read('ftp://cdsarc.u-strasbg.fr/pub/cats/J/AJ/152/157/table1.dat')
print(SPARC['MHI'].to(u.solMass))

astropy-bot · 2018-04-04T15:13:30Z

Hi there @kyleaoman 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

Everything looks good from my point of view! 👍

If there are any issues with this message, please report them here.

astrofrog · 2018-08-04T12:39:37Z

@kyleaoman - can you add the example you mention as a unit test for the CDS reader?

kyleaoman · 2018-08-07T11:25:16Z

Added a test. I merged changes from astropy/master in my branch a while ago, which I now see from the instructions was a bad idea.

kyleaoman · 2018-08-07T11:59:24Z

The change to the cds unit parsing seems to break some other tests, this will need to be looked into...

bsipocz · 2018-08-07T12:36:17Z

@kyleaoman - If you could rebase that would be the way to get rid of the merge issues. I'm happy to help with it if needed, just ping me on slack.

kyleaoman · 2018-08-07T16:02:56Z

@astrofrog added the test, checked that it fails before and passes after. Also cleaned up a bit with help from @bsipocz . There are a couple of other tests that now fail, which I'm having some trouble diagnosing. If one of you can help, or can ping someone else who can, that would be much appreciated.

Corrects a failed test after changes in PR astropy#7348. The CDS specification accepts both 'arcs' and 'arcsec' for arcseconds (and similarly for arcminutes), see http://vizier.u-strasbg.fr/cgi-bin/Unit. 'arcsec' seems to be the preferred form (http://cds.u-strasbg.fr/doc/catstd-3.2.htx, but an argument could be made the other way via 'rule 2' on that page) so make that the default string representation.

kyleaoman · 2018-08-08T13:30:39Z

Seem to have sorted out the issues with failing tests. Looks like all that's missing is a review from @taldcroft.

mhvk · 2018-08-09T14:15:18Z

                    col.unit = None  # "---" is the marker for no unit in CDS table
+                else:
+                    try:
+                        col.unit = Unit(unit, format='cds')


I'm not sure we should fail on the unit not parsing correctly - that forces the user to edit the files they downloaded. Instead, I would suggest to follow the fits example and just add parse_strict='warn' or even parse_strict='silent' (which means one get UnrecognizedUnit after all, but of course only for those units that are, in fact, not recognized).

But best to cc @taldcroft, since he will have a better idea about what the standard is in io.ascii (perhaps there is a keyword argument already for it??).

kyleaoman · 2018-08-20T08:17:49Z

I agree that a warning makes more sense than an error if a unit fails to parse. A user can always suppress a warning if they want it to be silent. @taldcroft hopefully this is ready for merge now?

kyleaoman · 2018-08-20T17:21:03Z

Hm, this 'coverage/coveralls' check is new to me... what exactly should be done to fix that?

taldcroft · 2018-08-20T20:22:21Z

-        assert iers_tab['UT1_UTC'].unit is u.second
-        assert iers_tab['PM_x'].unit is u.arcsecond
-        assert iers_tab['PM_y'].unit is u.arcsecond
+        assert (iers_tab['UT1_UTC'].unit / u.second).is_unity()


Humm, this is slightly disconcerting. I see the nice commit description that points to why this change was made. For example:

In [5]: u.s is u.Unit('s') Out[5]: True In [6]: u.s is u.Unit('s', format='cds') Out[6]: False In [7]: u.s / u.s Out[7]: Unit(dimensionless) In [8]: u.s / u.Unit('s', format='cds') Out[8]: Unit("s / s")

I had imagined that the unit format implies how to parse the unit string (or combinations thereof), but that the fundamental units coming out were the same thing. Not so apparently?

So this does imply some risk of breaking code with this PR. Thoughts @mhvk?

taldcroft · 2018-08-20T20:33:03Z

@kyleaoman - I think for this change it is not necessary to include the two new (large-ish) test files. You could instead just modify the following one to set a unit of GMsun for the Fit column.

https://github.com/astropy/astropy/blob/master/astropy/io/ascii/tests/t/cds.dat#L27

You should rebase away those files (basically git rebase -i .. and squash away 9701dc9, then re-add your new test using the existing (but modified cds.dat). That way you can keep all the original commits and messages. Let me know if this isn't clear.

taldcroft · 2018-08-20T20:40:08Z

As noted by the bot, you need to put in a changelog entry for this. I am labeling this as a bug fix because CDS units should be correctly parsed by the CDS reader, so the entry would go in the bug fix section.

However, it is not obvious to me that this is fully backwards compatible because of the issue with units, so as it stands this should NOT be backported.

taldcroft · 2018-08-20T20:41:15Z

@kyleaoman - apart from the little technical issues, this looks good, thanks for the contribution!

mhvk · 2018-08-21T08:21:01Z

That cds s is not the same as regular s is really annoying, and should be fixed! I am traveling so don't immediately have time to investigate, but:

In [4]: u.s / u.Unit(str(u.Unit('s', format='cds')))
Out[4]: Unit(dimensionless)

@taldcroft - could you raise a new issue?

taldcroft · 2018-08-21T10:08:44Z

@mhvk - OK, issue raised in #7751. If that can fixed then this PR would truly be back-compatible and could presumably be back-ported (assuming the #7751 fix could be back-ported as well).

Corrects a failed test after changes in PR astropy#7348. The CDS specification accepts both 'arcs' and 'arcsec' for arcseconds (and similarly for arcminutes), see http://vizier.u-strasbg.fr/cgi-bin/Unit. 'arcsec' seems to be the preferred form (http://cds.u-strasbg.fr/doc/catstd-3.2.htx, but an argument could be made the other way via 'rule 2' on that page) so make that the default string representation.

kyleaoman · 2018-08-21T12:03:02Z

@taldcroft Alright, think all the requested changes are handled, and everything checks out on the automated tests now.

taldcroft · 2018-08-21T14:29:12Z

                                   't/fixed_width_2_line.txt',
                                   't/cds/description/ReadMe',
                                   't/cds/description/table.dat',
+                                   't/cds/units/ReadMe',


This needs to be reverted.

Corrects a failed test after changes in PR astropy#7348. The CDS specification accepts both 'arcs' and 'arcsec' for arcseconds (and similarly for arcminutes), see http://vizier.u-strasbg.fr/cgi-bin/Unit. 'arcsec' seems to be the preferred form (http://cds.u-strasbg.fr/doc/catstd-3.2.htx, but an argument could be made the other way via 'rule 2' on that page) so make that the default string representation.

Comparisons fail with some units parsed from strings, some examples are below. I think the last should be preferred. u.Unit('s') is u.Unit('second') # True u.Unit('s') is u.Unit('sec', format='cds') # False u.Unit('s') is u.Unit('sec', format='cds').represents # True u.ms is u.Unit('ms', format='cds').represents # False (u.ms / u.Unit('ms', format='cds')).is_unity() # True

Added a bogus unit to an existing sample CDS format file to test parsing of cds-formatted units. Adjusted unrelated test which uses the same file.

kyleaoman · 2018-08-22T08:28:22Z

@taldcroft and done.

taldcroft

Thanks @kyleaoman ! Definitely appreciate getting help here to get the details just right.

Corrects a failed test after changes in PR astropy#7348. The CDS specification accepts both 'arcs' and 'arcsec' for arcseconds (and similarly for arcminutes), see http://vizier.u-strasbg.fr/cgi-bin/Unit. 'arcsec' seems to be the preferred form (http://cds.u-strasbg.fr/doc/catstd-3.2.htx, but an argument could be made the other way via 'rule 2' on that page) so make that the default string representation.

bsipocz added the io.ascii label Apr 4, 2018

bsipocz added this to the v3.1 milestone Apr 4, 2018

astrofrog requested a review from taldcroft August 4, 2018 12:38

Parse CDS units with Unit 'cds' format.

4a196f0

kyleaoman force-pushed the cds-tables-units branch 2 times, most recently from 53eddbc to 9701dc9 Compare August 7, 2018 15:04

mhvk reviewed Aug 9, 2018

View reviewed changes

taldcroft reviewed Aug 20, 2018

View reviewed changes

taldcroft added units Bug labels Aug 20, 2018

taldcroft mentioned this pull request Aug 21, 2018

CDS units are not the same as regular units #7751

Open

kyleaoman force-pushed the cds-tables-units branch from 9dff809 to 5dc98f0 Compare August 21, 2018 11:13

kyleaoman force-pushed the cds-tables-units branch from 5dc98f0 to 28d5a15 Compare August 21, 2018 11:17

taldcroft reviewed Aug 21, 2018

View reviewed changes

kyleaoman added 6 commits August 21, 2018 16:37

Added test for correct reading of CDS formatted units.

6ef6840

Warn instead of raise when parsing cds unit fails.

f22e2c4

Adjust comparison for cds header test.

5b0db1c

Added a bogus unit to an existing sample CDS format file to test parsing of cds-formatted units. Adjusted unrelated test which uses the same file.

Added changelog entry.

bd67f66

kyleaoman force-pushed the cds-tables-units branch from 28d5a15 to bd67f66 Compare August 21, 2018 14:38

taldcroft approved these changes Aug 22, 2018

View reviewed changes

taldcroft merged commit 7fe4970 into astropy:master Aug 22, 2018

kyleaoman deleted the cds-tables-units branch August 22, 2018 11:30

Conversation

kyleaoman commented Apr 4, 2018

Uh oh!

astropy-bot Bot commented Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astrofrog commented Aug 4, 2018

Uh oh!

kyleaoman commented Aug 7, 2018

Uh oh!

kyleaoman commented Aug 7, 2018

Uh oh!

bsipocz commented Aug 7, 2018

Uh oh!

kyleaoman commented Aug 7, 2018

Uh oh!

kyleaoman commented Aug 8, 2018

Uh oh!

mhvk Aug 9, 2018

Choose a reason for hiding this comment

Uh oh!

kyleaoman commented Aug 20, 2018

Uh oh!

kyleaoman commented Aug 20, 2018

Uh oh!

taldcroft Aug 20, 2018

Choose a reason for hiding this comment

Uh oh!

taldcroft commented Aug 20, 2018

Uh oh!

taldcroft commented Aug 20, 2018

Uh oh!

taldcroft commented Aug 20, 2018

Uh oh!

mhvk commented Aug 21, 2018

Uh oh!

taldcroft commented Aug 21, 2018

Uh oh!

kyleaoman commented Aug 21, 2018

Uh oh!

taldcroft Aug 21, 2018

Choose a reason for hiding this comment

Uh oh!

kyleaoman Aug 21, 2018

Choose a reason for hiding this comment

Uh oh!

kyleaoman commented Aug 22, 2018

Uh oh!

taldcroft left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

astropy-bot Bot commented Apr 4, 2018 •

edited

Loading