Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow Quantity to be written as a normal column to VOTable solves #3685 #6132

Merged
merged 5 commits into from Jun 1, 2017

Conversation

@aaryapatil
Copy link
Contributor

aaryapatil commented May 30, 2017

Work in Progress:
This works perfectly for write operation. For reading, round-tripping is maintained, except for the case where the unit is 'Angstrom'. For most of the other units, I have checked that it works perfectly well. Will have to investigate why 'Angstrom' is causing a problem.

In [34]: t
Out[34]: 
<QTable length=3>
   a       b       c    
  deg      s    Angstrom
float64 float64 float64 
------- ------- --------
    1.0     1.0      1.0
    2.0     2.0      2.0
    4.0     4.0      4.0

In [35]: t.write("check_votable", format='votable', overwrite=True)

In [36]: QTable.read("check_votable", format='votable')
Out[36]: 
<QTable masked=True length=3>
   a       b       c   
  deg      s       AA  
float64 float64 float64
------- ------- -------
    1.0     1.0     1.0
    2.0     2.0     2.0
    4.0     4.0     4.0

#5910

@taldcroft @mhvk Any idea as to why it fails only for Angstrom?
I'm keeping this open to make sure that the changes don't get lost.

The travis failure seems unrelated.

@mhvk

This comment has been minimized.

Copy link
Contributor

mhvk commented May 30, 2017

@AustereCuriosity - it is not strictly a failure: AA is just another way to write Angstrom, and I checked that the columns before and after compare as equal. But I'm confused why it picks a different name in the two cases!

@bsipocz

This comment has been minimized.

Copy link
Member

bsipocz commented May 30, 2017

@AustereCuriosity - This needs to be rebased to get rid of the merge commits. I expect it to be a clean an easy one, but let me know if you run into troubles.

@@ -124,6 +124,9 @@ def test_io_quantity_write(tmpdir):
assert isinstance(qt['a'], u.Quantity)
assert qt['a'].unit == 'Angstrom'
continue
if fmt == 'votable':
t.write(filename, format=fmt, overwrite=True)

This comment has been minimized.

Copy link
@pllim

pllim May 30, 2017

Member

The test is not very convincing. There is no check whether it is written correctly or not.

CHANGES.rst Outdated
@@ -78,6 +78,9 @@ New Features
- Add support for Quantity columns (within a ``QTable``) in table
``join()``, ``hstack()`` and ``vstack()`` operations. [#5841]

- Added functionality to allow ``astropy.units.Quantity`` to be written
as a normal column to VOTable files. [#6132]

This comment has been minimized.

Copy link
@pllim

pllim May 30, 2017

Member

Nitpick: Either change "VOTable" to "VO table" or use double backticks on it.

@aaryapatil aaryapatil force-pushed the aaryapatil:issue_3685_votable branch 2 times, most recently from 9a8711f to 5023534 May 31, 2017
@aaryapatil

This comment has been minimized.

Copy link
Contributor Author

aaryapatil commented May 31, 2017

@bsipocz The rebase has been done.
@mhvk Indeed, it does compare as equal. The 'AA' was just a little misleading!
@pllim I've added the validation of the output to the tests and corrected the changelog entry.

I think this is mostly done. A final review maybe?

Copy link
Contributor

mhvk left a comment

@AustereCuriosity - now had a detailed look. I only have some small comments.

@@ -141,10 +143,12 @@ def write_table_votable(input, output, table_id=None, overwrite=False,

# Tables with mixin columns are not supported
if input.has_mixin_columns:

This comment has been minimized.

Copy link
@mhvk

mhvk May 31, 2017

Contributor

I think has_mixin_columns is not really needed any more, so would suggest just removing this if statement.

if unsupported_cols:
unsupported_names = [col.info.name for col in unsupported_cols]
raise ValueError('cannot write table with mixin column(s) {0} to VOTable'
.format(unsupported_names))

This comment has been minimized.

Copy link
@mhvk

mhvk May 31, 2017

Contributor

Indentation is not quite right, though if you remove the outer if statement, as suggested above, this will become OK.

@@ -1547,24 +1547,25 @@ def from_table_column(cls, votable, column):
`astropy.table.Column` instance.
"""
kwargs = {}
meta = column.info.meta or {}

This comment has been minimized.

Copy link
@mhvk

mhvk May 31, 2017

Contributor

I think here I'd prefer an if statement, so the loop does not get executed unless meta actually contains something, i.e.,

meta = column.info.meta
if meta:
    for key ...
field.values.from_table_column(column)
if 'links' in column.meta:
for link in column.meta['links']:
if 'links' in meta:

This comment has been minimized.

Copy link
@mhvk

mhvk May 31, 2017

Contributor

if we go with my above suggestion, this would change to

if meta and 'links' in meta:
    ...
@mhvk

This comment has been minimized.

Copy link
Contributor

mhvk commented May 31, 2017

Note: I canceled the build, since at present master is broken for python3 (by accident, @taldcroft promised a fix soon), so the tests would fail anyway. Given that appveyor passed on python2, your PR should be fine.

@aaryapatil

This comment has been minimized.

Copy link
Contributor Author

aaryapatil commented May 31, 2017

@mhvk I've done the changes. Have a look!

@mhvk

This comment has been minimized.

Copy link
Contributor

mhvk commented May 31, 2017

@AustereCuriosity - this looks all OK to me now.

The failure is unrelated, but I'd like to have this properly tested just in case. So, it will need a rebase after #6142 is merged (or an alternative to it). Unfortunately, travis seems pretty stuck... (I canceled your travis runs just to try to get it sped up a bit).

@bsipocz

This comment has been minimized.

Copy link
Member

bsipocz commented May 31, 2017

@mhvk - There was a github outage earlier today that messed up travis quite badly.

@mhvk

This comment has been minimized.

Copy link
Contributor

mhvk commented May 31, 2017

@AustereCuriosity - if you rebase on master now, the tests should be run fine.

@@ -118,7 +118,7 @@ def test_io_quantity_write(tmpdir):
open(filename, 'w').close()

for fmt in ('fits', 'votable', 'hdf5'):
if fmt == 'fits':
if fmt != 'hdf5':

This comment has been minimized.

Copy link
@taldcroft

taldcroft May 31, 2017

Member

I usually advocate minimal changes, but in this case the logic here has gotten really tortured for no good reason. Suggest:

    # Show that FITS and VOTable formats succeed
    for fmt in ('fits', 'votable'):
        t.write(filename, format=fmt, overwrite=True)
        qt = QTable.read(filename, format=fmt)
        assert isinstance(qt['a'], u.Quantity)
        assert qt['a'].unit == 'Angstrom'

    # Show that HDF5 format fails
    if HAS_H5PY:
         with pytest.raises(ValueError) as err:
             t.write(filename, format='hdf5', overwrite=True)
         assert 'cannot write table with mixin column(s)' in str(err.value)
Copy link
Member

taldcroft left a comment

Looks generally good to me with a couple of changes requested (and assuming it passes tests).

CHANGES.rst Outdated
@@ -87,6 +87,9 @@ New Features
Python 3 ``str`` object in a bytestring column (numpy ``'S'`` dtype).
[#5700]

- Added functionality to allow ``astropy.units.Quantity`` to be written

This comment has been minimized.

Copy link
@taldcroft

taldcroft May 31, 2017

Member

Suggest

... to be read
and written from a VOtable file. [#6132]
@aaryapatil aaryapatil force-pushed the aaryapatil:issue_3685_votable branch from a845825 to 744caf0 May 31, 2017
@aaryapatil

This comment has been minimized.

Copy link
Contributor Author

aaryapatil commented Jun 1, 2017

I rebased on master and the checks have now passed. The changes have also been done. Is this OK now?

@mhvk

This comment has been minimized.

Copy link
Contributor

mhvk commented Jun 1, 2017

@AustereCuriosity - looks all good to me. Since you implemented @taldcroft's one suggestion, I'll merge. Thanks!

@mhvk mhvk merged commit e91dbe3 into astropy:master Jun 1, 2017
4 checks passed
4 checks passed
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 84.419%
Details
@mhvk mhvk added the Affects-release label Jun 1, 2017
@aaryapatil aaryapatil deleted the aaryapatil:issue_3685_votable branch Jun 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.