Python 3 support

[montefra]

Hi Eric,

This PR adds support for python 3.

I've tried very hard:

• not to break the python 2 way
• not to change the functions/methods signature, to allow seamless switching python 2 <-> 3: in python 3 xpa uses byte string, but all the functions/methods in pyds9.py get and return standard strings
• not to break the code: for both python 2 and 3 I've successfully run python pyds9.py; besides I've tested creating new frames, adding new files and setting/getting a region.

I also took the liberty to make some of the code compliant with PEP8.

Cheers

[montefra]

I forgot: The code is tested on python 2.7 and 3.4 on Linux (opensuse).

If from __future__ import print_function works on python 2.6, than I think that the code should work there. I wouldn't be surprised if the code works on python 3.3 and 3.2

[ericmandel]

@montefra This is fantastic and much appreciated. I confess that I know nothing about python3, so I propose to ask potentially interested parties: @olebole, @cdeil, @eteq, @timstaley, @sosey to make comments. But I really appreciate the contribution.

Would you consider doing the same for pyjs9?

[montefra]

@ericmandel : thanks.

I might do one or more commits to improve the setup.py file and to update the installation instructions to use pip as preferred way.

[ericmandel]

Yes, please do. As I said, I have to rely on you Python experts to decide how to do things in the right way. I'm just trying to make sure we (DS9 and JS9) can support your efforts.

[cdeil]

@montefra – Thanks!

It's very hard to review such PRs by reading the diff ... one would have to manually try excercising all the code with Py 2 / 3 and see if it works. This is very time consuming and has to be re-done (partly) for every change.

I realise it might be hard for pyds9 to test the GUI parts, but isn't there a way to set up limited testing on travis-ci (in a separate PR)?

[ericmandel]

@cdeil @montefra Actually, I didn't have in mind testing but rather, that you Python experts agree on the approach taken. My outsider impression is that "the right way" is still evolving, so I wanted to give you the opportunity to comment on that side of things.

Otherwise, my inclination would be to check the python2 support and then merge it.

[Aside: so far as automatic GUI testing is concerned, after our spring conversations I developed a pretty decent local system using selenium for js9, but what I found is that the important tests are really hard to write. In the end, I don't feel the need to test whether the colormap command really changes the displayed map from "red" to "green". But I do need to test that a region is "the same" when the image is binned by 2 and then zoomed by 2 (inverse operations) -- and this sort of quantitative test is hard.]

[montefra]

@cdeil : the PR has some change of syntax to run both on python 2 and 3, a lot of PEP8 (I'm fixated with it). The core changes are because xpa communicates using byte strings. In python 2 strings and byte strings are the equal (and unicode is an other type) while in python 3 there strings and byte strings are different types, so an explicit conversion is necessary.

If its possible to emulate a ds9 session to which xpa talks (just listening and replying), then it's probably not too complicated to write some test for pyds9.

@ericmandel : I might revise pyds9.py in the next days to see if there are better/more general ways to deal with string conversion.

[montefra]

@ericmandel: I'll try to follow the suggestions here.

[ericmandel]

This is all great work, thanks! I think you should take your time as needed. I am in no hurry on my end and in fact, I really can't put any serious time into this until Sep 17, earliest.

[ericmandel]

If its possible to emulate a ds9 session to which xpa talks (just listening and replying), then it's probably not too complicated to write some test for pyds9.

@montefra The pyds9 set and get methods are just thin layers on the XPA set and get primitives (unlike the richer set of pyjs9 routines), so you don't actually need DS9 functionality in an emulator -- any public access point(s) would do. So perhaps the stest.c (server test) program in the xpa sub-directory would suffice. We can talk about this later ...

[ericmandel]

@ericmandel: I'll try to follow the suggestions here

(where here is: https://docs.python.org/3/howto/pyporting.html#text-versus-binary-data)

This might be a problem for pyds9. The 'buf' argument in both the set() and get() methods can contain either text (I think DS9 assumes ASCII text) or binary (usually FITS data or binary arrays). For example the "regions" access point sends/returns text, while the "fits" access point sends/returns binary.

[montefra]

@ericmandel: that's true in python2 where strings and bytes are the same type. So you can have standard strings buf and fits arrays.

In python 3 strings and byte strings are two different types and ctype.c_char_p accepts either integers or bytes. So for any communication with xpa I first encode the input, if a string, and then decode the output like here.

[ericmandel]

What happens here in get() if the returned x data is a FITS file instead of a string? It looks like bytes_to_string() will try to decode each byte of the binary FITS data ... which doesn't sounds right, to the uninitiated ...

[montefra]

xpaget and xpaaccess return lists of strings (python 2) or list of bytes (python 3) according to here and here, so bytes_to_string convert every element of the list from bytes to string.

[montefra]

I've also created a casa.fits file and rerun the test. On my opensuse installation, with ds9 v 7.3.2 the test runs through with master and with the python3 branch both on python 2 and 3.

But today I was trying to run the same on a Kubuntu installation on the same machine, with ds9 v7.2, and it was failing to run the tests with the casa.fits file in most of the cases. The call to DS9('pytest2') did open a ds9 window with the correct title but then I get:

ValueError, no active ds9 running for target: pytest2

Keeping the window open, upon rerunning the test I did get this long message. I was able to run the test once with python 2, but than it failed multiple times with the same settings. However in some of my attempts I was able to create the pytest2 window and send a file to it.

[ericmandel]

ValueError, no active ds9 running for target: pytest2

This sounds like a timeout problem. The python code waits for the connection to be made (default is 10 seconds, I think) but eventually gives up. Note that having DS9 itself up and running is not enough, there also has to be an initial socket connection made (i.e. that DS9 is actually ready to receive xpa requests). And for this to happen, the xpans name server also has to be started (by default, this done by ds9 itself, if no active xpans is running). So you can experiment:

1. in DS9(), change the wait time to something longer or ...
2. start up xpans manually (its located somewhere as a result of the build and install, and you can just execute it at the command line) or ...
3. in another window, loop on a "ps guwax | egrep xpans" and see when it shows up or ...
4. in another window, loop on "xpaaccess -c ds9" to see when DS9 is ready to field XPA requests, if you have the xpaaccess command line program (might have to download from DS9 site) or ...
5. in another python window, load pyds9 and loop on executing xpa.xpaaccess

In all this, you're just trying to see when everything is ready ...

[montefra]

I hate to force the user have to remember to set decode to false when calling get() on "fits" or "array". [...] Otherwise, it's guaranteed that users will forget to do this, and accidentally end up with changed pixel values -- without knowing that it has happened. Since the "binary" access points are known, we can just do this automatically and avoid any confusion.

I see your point. And agree. The more we keep the python2 and 3 behaviour the same the better.

Do you think we should add to get() a list of access points for which the decode would be turned off?

Easy enough. We can change the call to

def get(self, paramlist=None, no_decode=["fits", "array", "mosaic", "mosaicimage"]):
    [...]
    if paramlist not in no_decode:
        x = bytes_to_string(x)
    [...]

This way the user can force/disable the decoding passing no_decode=[]/no_decode=[paramlist] or, with a little bit of logic, no_decode=False/no_decode=True

---

As now get_{arr2np,pyfits} turn the decode off, so they do the correct thing.

[ericmandel]

@montefra That's a nice way of doing it ... I'll pulse Bill Joye, DS9 author, to look at the public access points to make sure we have all of the current ones that return binary data.

One minor point: you'll want to check the first token of the paramlist, not the whole paramlist: the access point name is the first token, after which comes optional qualifiers (it's like a mini language). The token delimiter can be space or tab.

[montefra]

Ok, I'll wait. And thanks for bearing with me. I'm not a ds9 power user, and I embarked in the project because I've interest in using pyds9 in a project that I want to be python2 and 3 compatible.

One minor point: you'll want to check the first token of the paramlist, not the whole paramlist: the access point name is the first token, after which comes optional qualifiers (it's like a mini language). The token delimiter can be space or tab.

Would do you expect string or bytes from this?

self.get('fits width')

I would say string, so this should be decoded. But I would expect bytes from self.get('fits').

Is there a list somewhere with all the possible optional qualifiers for fits and array, so that I can have an idea of how many paramlist shouldn't be decode?

[ericmandel]

@montefra

And thanks for bearing with me.

You have no idea how happy I am to have this help. I wrote pyds9 and pyjs9 in recognition of the importance of the Python community to astronomy, but not being an expert, I really can't support without you. So thanks.

But I would expect bytes from self.get('fits').

Yes, it's a little complicated, but here is the list, direct from Bill Joye, of the "binary" commands we have to deal with:

array
fits
fits image
fits table
fits slice
gif
jpeg
mecube
mosaic
mosaicimage
nrrd
png
rgbarray
rgbcube
rgbimage
tiff

Since Bill has provided the complete list, you can just check the full paramlist as you originally were going to do. No need to test the first token (which was my feeble attempt to deal with qualifiers).

Given the length of the list, do you still think it best to pass the list in the 3rd (no_decode) arg, as opposed to putting it inside the module? I am not advocating either way, since I don't know the best way to present a Python method. What I am wondering is what someone would do if (s)he wanted to add to the list? Might it be preferable to have add a property to the ds9Gobals object called "no_decode" to contain this list of access points? Then someone who wanted to change the list would simply edit that property. Just a thought.

[montefra]

@ericmandel: here it is.

In self.get method if decode is:

• True: decode the output of xpa
• False: don't decode the output of xpa
• None: rely on ds9Globals['bin_cmd'] list to decide if do the decoding or not

[ericmandel]

This looks very good indeed. Perhaps the third option should be "Auto" to make more clear what happens in that case?

Oh wait ... is None the default in cases where the decode option is not passed at all? If so, then that is a good way of doing it, better than "Auto".

[montefra]

Oh wait ... is None the default in cases where the decode option is not passed at all? If so, then that is a good way of doing it, better than "Auto".

yes.

It's common use in python to use None for options that should default to something. If the default is a non-mutable object, like a string or number, the default is often set to it. But if the default is a mutable (list, dictionary) or needs to be computed at run-time (like e.g. a port number), then the default is set to None.

[eteq]

To answer the original question asked by @ericmandel : I think this is the right approach for cross-compatibility: I see @montefra is using six along with the from __future__ import print_function approach, which is exactly how I do it in code I'm trying to go both ways on.

I agree with @cdeil, though, that it's very difficult to check this kind of code for subtle bugs without some sort of testing suite that can be run in both, though. Obviously @montefra has thought a lot about the bytes/str problem, but this stuff can be very tricky... And there are other subtle gotchas like the iteritems/iter behavior change from 2.x->3.x. I don't think that crops up here, but it's an example of subtle things that you can miss if you're not very careful. To be honest, I think it's more important for future changes - if someone else contributes who doesn't know as much about py 2/3, they may well just miss that they have to use six or whatever...

[montefra]

To answer the original question asked by @ericmandel : I think this is the right approach for cross-compatibility: I see @montefra is using six along with the from future import print_function approach, which is exactly how I do it in code I'm trying to go both ways on.

Thanks!

I agree with @cdeil, though, that it's very difficult to check this kind of code for subtle bugs without some sort of testing suite that can be run in both, though. Obviously @montefra has thought a lot about the bytes/str problem, but this stuff can be very tricky...

This is true. But subtle bugs are hard anyway and a testing suit would be valuable independently of the language/versions of language/etc. Testing by hand is tedious at best... Btw: in pyds9.py there one function that runs some test. I've used this to check that I get the same in py2 and py3. This could be the beginning for a proper test suite.

[cdeil]

IMO the most efficient way forward is to merge this when @montefra thinks it's ready (and maybe @ericmandel has done some manual testing that it still works on Python 2) and then we continue with setting up tests in other PRs. As a start I've opened #15 to split out the discussion on testing from this PR.

[montefra]

@cdeil :+1: Thanks for taking care of it.

I'll check the code according to your suggestions and then I guess that for now is done. We can fix issues when we write tests.

[ericmandel]

@cdeil @montefra @eteq It's been nearly six years since I wrote pyds9 and I believe we have come to a point where the Py activity is starting to dominate the DS9 part. The p2/p3 conversion persuades me that the active issues of development and maintenance are going to center around Python, not DS9. I therefore invite one or more of you to become collaborators in the pyds9 repository, so that you can make changes as needed and accept PRs as warranted. Of course I want to be involved in questions related to DS9 itself, and also in questions related to the API interface (e.g. should we extend the methods in the way pyjs9 does?). But it's silly for me to merge pull requests that I barely understand and have to ask how to deal with travis-cl.

If you think this is a good idea, I suggest we start by adding one person, which you may want to decide among you. But if you decide it's best to have more than one, that's fine too.

My suggestion is further acknowledgment of the importance of the Python community in astronomy, as was my initial implementation of pyds9 (and pyjs9).

[cdeil]

@ericmandel - I vote for @montefra to become co-maintainer here, because ... see this PR!

[olebole]

:thumbsup: for @montefra

[montefra]

Thank for the support. I can be co-maintainer for the python part. I have no idea about the cpart and ctypes (but i guess that @ericmandel is our expert there). I have not much experience in maintaining a publicly used package but this seems a good starting point.

[ericmandel]

@montefra @cdeil @olebole @eteq

Welcome @montefra as a collaborator! I am sure we will find a natural division of labor that will greatly benefit the pyds9 project.

I think you have PR merge permission but if not, I can merge the current PR and we then move on from there. Let me know ...

[montefra]

@ericmandel I have the permission. Thank you for it. I feel more important now :+1: Before merging: do you think that I should modify something else? I've updated a bit the readme and the change log file.

I've noticed that there is a index.rst with some info that looks to me the same as in the read me file. I would address documentation and install instruction in a separate PR, though

[ericmandel]

I feel more important now :+1:

If the SAO R&D group (which produced SAOtng, ASSIST, XPA, funtools, and DS9) still existed, you'd be an all-the-rights-possessing member!

The pyds9 project was started a few years before it went onto GitHub, so the files you see are probably not quite standard. I'd recommend straightening it out separately.

[cdeil]

:tada: Thanks!

[ericmandel]

Very nicely done!

[montefra]

a pleasure!

[ericmandel]

@montefra one more change, please ... In the README, change:

Contact saord@cfa.harvard.edu for help.

Eric Mandel

to:

Please contact us at https://github.com/ericmandel/pyds9/issues if you need help.

Eric Mandel and Francesco Montesano