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

Table's subclass break with serialization #7181

Closed
saimn opened this issue Feb 12, 2018 · 5 comments
Closed

Table's subclass break with serialization #7181

saimn opened this issue Feb 12, 2018 · 5 comments

Comments

@saimn
Copy link
Contributor

saimn commented Feb 12, 2018

With the serialization of astropy objects to FITS (#6912), I just noticed that it breaks the use of our Table subclass. When writing and reading back a FITS file with this subclass, io.registry.read now fails because the reader return a QTable, for which my class is not a subclass.
I don't see a way to disable the serialization, or to not use it while reading. Any idea @taldcroft ?

@saimn
Copy link
Contributor Author

saimn commented Feb 12, 2018

Actually the failure is because of #7147, and because we set .format and .description on our columns (to save both as FITS and VOTable).

@pllim
Copy link
Member

pllim commented Feb 12, 2018

(Ops, I kept adding a milestone by habit... I removed it. Ignore the confusing history there.)

@taldcroft
Copy link
Member

taldcroft commented Feb 12, 2018

Here is a demonstration of the problem:

from astropy.table import Table
import astropy.units as u

class MTable(Table):
    pass

mt = MTable([[1, 2.5]], names=['a'])
mt['a'].unit = u.m
mt['a'].format = '.4f'
mt['a'].description = 'hello'

mt.write('junk.fits', overwrite=True)
mt2 = MTable.read('junk.fits')

Here is a potential fix. Basically it just punts on making sure that the object the reader returns is a strict subclass of the output class, and instead just tries converting. This works for Table/QTable. Not sure if removing this test is a worry for other reader classes. @astrofrog ?

(astropy) neptune$ git diff
diff --git a/astropy/io/registry.py b/astropy/io/registry.py
index b332f83..d9df055 100644
--- a/astropy/io/registry.py
+++ b/astropy/io/registry.py
@@ -517,18 +517,15 @@ def read(cls, *args, format=None, **kwargs):
         data = reader(*args, **kwargs)
 
         if not isinstance(data, cls):
-            if issubclass(cls, data.__class__):
-                # User has read with a subclass where only the parent class is
-                # registered.  This returns the parent class, so try coercing
-                # to desired subclass.
-                try:
-                    data = cls(data)
-                except Exception:
-                    raise TypeError('could not convert reader output to {0} '
-                                    'class.'.format(cls.__name__))
-            else:
-                raise TypeError("reader should return a {0} instance"
-                                "".format(cls.__name__))
+            # User has read with a subclass where only the parent class is
+            # registered.  This returns the parent class, so try coercing
+            # to desired subclass.
+            try:
+                data = cls(data)
+            except Exception:
+                raise TypeError('could not convert reader output to {0} '
+                                'class.'.format(cls.__name__))
+

I'm supposed to be on holiday this week so if anyone else wants to pick this up that would be good. 😄

@taldcroft
Copy link
Member

BTW see line below for the source of the QTable, and see #7190 for discussion related to why it needs to be QTable at that point:

# Don't know final output class but assume QTable so no columns get

@saimn
Copy link
Contributor Author

saimn commented Feb 13, 2018

Thanks for the quick answer and the patch @taldcroft , I can have a look to integrate this with a test if you want to enjoy your holidays 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants