FITS_rec objects can be pickled with this patch

[bsipocz]

This is a PR to fix issue #1597. It seems working to me (pickled a few different fits tables with it), but please advise how it should be tested properly.

[astrofrog]

Thanks for the fix! @embray can review and provide advice about the test.

[embray]

I'll have to take a closer look at this. I feel like the solution may be simpler than this but I don't know for sure.

[bsipocz]

To be honest I thought it was an easy patch to do, but it took me much longer to figure out a way to fix it that I thought.
I'm still sure there is a nicer way to do it, but deep-copying _coldefs was the simplest I could come up with.

[embray]

Sorry I've been slow to respond on this--I have to say it looks good minus a few trivial nitpicks. I just don't fully understand the issue or how exactly this fixes it and need to find time to take a closer look. Hopefully I'll get to that soon since this is very much a bug that needs fixing, and your PR is probably mostly good!

[embray]

This could be rewritten:

for attr, value in zip(meta, column_state):
    setattr(self, attr, value)

to me that's a little clearer as to what's going on.

[embray]

Thanks, this seems to solve the problem fairly straightforwardly and (I think--I still haven't investigated deeply) correctly. Please address the notes I made in the code and please also add some regression tests in the astropy.io.fits.tests.test_table module. Be sure to reference the issue number in the test docstring following the format of the other tests there.

There are already some sample tables included in the test modules that can be accessed via self.data(...) (again just follow the examples of the other tests). The should try pickling and unpickling several tables and comparing the results. Try also pickling a table that hasn't actually been read yet (the case where the column arrays are Delayed--it's good that you already accounted for that case but it should have a test).

Thanks!

[bsipocz]

Thanks for the review @embray. I try to clean this up and add tests later this week.

[bsipocz]

I have a slightly related question. Do we want to pickle HDUList objects as well? I can't think any case when pickling the whole HDUList, rather than just parts of it, is the optimal way.

[embray]

Not entirely sure how I feel about including 'parnames' here But on the same token, nobody outside of PyFITS itself is expected to ever subclass FITS_rec so I'm okay with it for now, especially considering the intention to eventually remove it anyway.

[embray]

Just needs one small update to the tests and a changelog entry and should be good to go--thanks!

[astrofrog]

@bsipocz - it looks like this will need rebasing - thanks!

[cdeil]

@bsipocz There's a test errors on Python 3. Can you have a look?
https://travis-ci.org/astropy/astropy/jobs/22703056#L1652

[embray]

Ah, I've run into this problem before with pickle on Python 3--this is due to the new new pickle protocol used as the default in newer Python versions. I believe the solution is to provide a __getnewargs__ method as well.

[bsipocz]

It looks similar to the problem in #2123.
If it's trivial to fix with a __getnewargs__() as suggested there, I try to fix it, but even then it may take some time (e.g. have to get a working python3 first...).

[cdeil]

@bsipocz Are you on Mac or Linux?
On Mac http://www.macports.org/ is a super-easy way to get multiple Python versions:

sudo port install py27-astropy py33-astropy

Some Linuxes also have python3 versions and allow parallel install via the package manager.

[bsipocz]

I should have checked the new comments first. Sorry for duplicating it @embray.

[embray]

That's okay--good to see you were already on it. But yeah, for development on Astropy I would definitely recommend at least having one Python 3 version available--preferably 3.3.

[bsipocz]

Thanks for the tips @cdeil .
I go with fink, which is not very up-to-date compared to macports, and thus it's a bit difficult to get a fully working environment out of it including a decent IDE, etc. Anyway I have a working 3.3 now, and with these 2 extra lines managed to pass the tests.

Let's see whether Travis likes it, too.

[cdeil]

@bsipocz I use the Eclipse editor with the PyDev plugin ... if you haven't found an IDE you like, I think it's worth trying it out.

[cdeil]

The travis-ci error is unrelated ... @embray or @astrofrog could you please restart the build?
https://travis-ci.org/astropy/astropy/jobs/22816314#L1383

Otherwise I think this is ready for final review by @embray.

[bsipocz]

I use eric4 for python2.7, but eric5 didn't build because of some non trivial dependency issues. I'll definitely sort it out for the long turn, I'm just short of time at the moment.

[embray]

Okay, I'm satisfied. Thanks @bsipocz !