ericmandel / pyds9

Python connection to SAOimage DS9 via XPA
76 stars 41 forks source link

Relation to `pysao` and `numdisplay`? #2

Open cdeil opened 11 years ago

cdeil commented 11 years ago

Hi @ericmandel, @leejjoon,

I think ds9 + Python interaction is very powerful and would like to use this / see it promoted more. I have a few suggestions for improvement for pyds9 (and would like to help as much as I can) like docs, tests, Python 3 ... but before putting effort into pyds9 I have a question.

There are two other ds9-Python packages on PyPI: numdisplay and pysao.

Would it make sense to join forces? For users having one well-documented, well-tested package with all features would be best. Or are there differences between pyds9, pysao and numdisplay so that it would make sense to have several packages?

@astrofrog @eteq @keflavich or other astropy guys might be interested.

keflavich commented 11 years ago

+1 I am interested and could potentially help some.

ericmandel commented 11 years ago

Hi @cdeil, @leejoon, and @timstaley

Thanks for making contact!

I believe numdisplay offers IRAF-like functionality to connect python to DS9 and XImtool, and as such, parallels the IRAF display tool, not the XPA connection. pysao is more akin to pyds9 in that it offers ds9.set and ds9.get to implement the XPA connection to ds9. But significantly, it also adds high-level functionality that pyds9 does not have (panto and panto_wcs, for example). Note that Tim Staley has also added high level functionality beyond pyds9 in his https://github.com/timstaley/pyds9_ex. And I also have received another contribution of similar high-level functionality created here at the Center for Astrophysics -- I've not had time to do anything with it.

Therein lies the problem for us DS9/JS9 developers. We built the basic python/DS9 connection into pyds9, keeping close to the XPA functionality implemented in DS9. (We are responsible for both DS9 and XPA.) But we didn't feel like we could decide between the different offerings of high-level functionality -- even as we readily acknowledge a need for it. (And frankly, we also don't have the resources to put into development of high level functionality, since DS9 and JS9 are pretty intensive efforts.) Getting community involvement on this sort of issue is the exact reason we put pyds9 into github.

In my opinion, what would make sense would be to 1) improve pyds9 as a core package (I appreciate help, since python is not my primary focus) and 2) develop an add-on package of high-level routines -- some combination of pysao, pyds9_ex, and whatever else the community comes to agree upon. Of course, if after some progress is made, it seems more logical combine everything into one package, I'm certainly open to the idea. But I believe there are two different efforts here, at least to start.

Regards,

Eric

eteq commented 11 years ago

@ericmandel - I'm not sure I fully undertsand your final comment here. It sounds to me like both pyds9 and pysao separately implement the XPA link, is that right? So then why do there need to be two different efforts? Perhaps the think to do is split into two packages that are "low-level" and "high-level" instead? That seems like a more effective use of resources, because no one would need to learn two different low-level interfaces to implement a high-level layer.

Or is there some subtlety I'm missing?

ericmandel commented 11 years ago

Sorry for the confusion: I am indeed talking about a low-level effort (pyds9) and a high-level effort (worked out by the community with examples such as pysao and pyds9_ex). The latter would be based on the former.

cdeil commented 11 years ago

@ericmandel Thanks for all the explanations and this effort to improve the Python-DS9 interop.

@ericmandel @leejjoon and @timstaley and anyone else interested: How about scheduling a Google hangout in the near future to have a chat?

Before that it would be nice to summarise what pyds9, pysao and pyds9_ex and other high-level interfaces users have created do exactly and to list questions and suggestions how to proceed. @ericmandel Can you please activate the wiki pages for pyds9 on github ... this seems like the best place to gather such info.

In the meantime I'll put some info here (please comment if anything is wrong):

Here's some questions I have about the low-level part:

About the high-level part we can be discussed later, just one comment:

Sorry for the long text ... I'm just a user and will shut up now ... time for the devs (@ericmandel and @leejjoon) to say what they think should be done.

timstaley commented 11 years ago

Hi all, just thought I'd post to say I'm watching with interest. Let me know if there is to be discussion elsewhere and I'll try my best to make it. Cheers, Tim.

ericmandel commented 11 years ago

I enabled with wiki and took the liberty of adding @cdeil's last comment to the "python community development" page. I'll respond on the wiki ...

ericmandel commented 11 years ago

On second thought, I'll respond to two points here: I agree that the underlying part of pysao overlaps the function of pyds9 and only one is needed. I am not a python expert, so I cannot comment on whether ctypes or Cython is preferable. I will say that we are willing to continue support for pyds9 as part of the ds9 project, but we are amenable if the community wants to move to pysao instead -- but we probably will be less involved with its direct support.

Regarding high-level routines: my assumption has been that there will be many flavors and it will be hard to decide which to make "official". This is the reason we are hesitant to add high-level routines directly to the core pyds9 module. So an extension index could be the right way to go.

eteq commented 11 years ago

@ericmandel - this is of course at some level personal preference, but I think Cython is slightly preferable from the python side of things. I find that it's generally easier to understand what's going on because you are actually writing code that parallels how you might actually write the C-code, and is thus quite readable. Whereas reading ctypes code really requires understanding the ctypes API. There's also some difference in setup.py that I think is somewhat streamlined in Cython, but I don't really know how much that matters here.

That said, once it's working (which they both are), that only matters if you want community involvement extending the low-level code. My perspective is just that it's better to have only one low-level interface unless the two interfaces have two different use-cases (which doesn't seem to be the case here, but I admit I haven't looked too closely).

cdeil commented 11 years ago

I just realized I misspelled @leejoon instead of @leejjoon. Sorry!

@leejjoon What do you think?

cdeil commented 11 years ago

@ericmandel I think Github issues are better for discussions, and wiki pages to document the conclusion. So if you don't mind I'll rewrite https://github.com/ericmandel/pyds9/wiki/pyds9-community-development in this way and then we can all iterate on it (and hopefully have a quick Google hangout about it soon online).

cdeil commented 11 years ago

@eteq Concerning the question whether ctypes or Cython is better for the low-level XPA wrapper. In this case it's so simple (less than 200 lines of code) that I think ctypes is better simply because it doesn't need a compiler on the user machine, i.e. is easier for some users to install. There's a few potential issues I don't know about that would help in the decision:

eteq commented 11 years ago

@cdeil Hmm, the code in pyds9's setup.py looks to me like it requires a compiler... But if that's relaxed (perhaps by linking to an existing shared library), then I see your point.

cdeil commented 11 years ago

@eteq I think (but might be wrong) the compiler in pyds9's setup.py is only used to build the XPA C library (now on github here, the ctypes Python interface to XPA doesn't require a compiler.

Both the pyds9 and pysao repos contain a copy of the XPA library ... which is much larger than the XPA Python wrapper code or Python ds9 interface code.

@ericmandel Maybe you can briefly explain the situation?

cdeil commented 11 years ago

I don't have time this week ... but I plan to have a closer look at the functionality / code / API in pyds9 and pysao next week.

ericmandel commented 11 years ago

@cdeil -- yes, please rewrite the wiki as you see fit to get us better organized ...

@eteq -- The XPA story is this: the XPA messaging system (built in the 1990s, more or less frozen, but still supported) is used to communicate with ds9 and also with other server programs (e.g. Chandra CIAO software, MIT ISIS etc). The pyds9 connection also uses XPA to talk to the rich set of XPA access points in ds9. Since we supply pyds9 via tar file from the CFA Web site, we package XPA into the pyds9 tar file and arrange for setup.py to build the XPA libraries and programs from source (hence the need for a C ompiler) as part of the installation. If you clone pyds9, you also need to build XPA, hence its inclusion in the pyds9 repository. The XPA repository is for other projects using XPA and also for maintenance. If someone knows how to clone pyds9 in such a way that the XPA repository also is retrieved at the same time, that might be a good thing ...

I don't think it will be easy to reproduce XPA functionality in python, but I'm open (though I don't have time or inclination to work on a new messaging system ... unless and until I get a chance to work with zeromq).

eteq commented 11 years ago

If someone knows how to clone pyds9 in such a way that the XPA repository also is retrieved at the same time, that might be a good thing ...

This is precisely what git submodules are for, actually. They're slightly non-trivial to use, though.

I don't think it will be easy to reproduce XPA functionality in python,

Oh, I definitely wasn't suggesting that. I think what @cdeil was saying is that if the user already has XPA installed as a shared library, ctypes can use that without anything ever having to be compiled (unlike Cython, which has to be compiled because it generates C code). But then the user has to get the XPA shared library somewhere.

cdeil commented 11 years ago

On Macports I see that XPA and pyds9 are separate packages: xpa portfile and the py-pyds9 portfile Although in Macports py-pyds9 does not depend on xpa ... it just creates it's own libxpa.dylib ... I presume because currently SAO is bundling XPA and the Python wrapper with pyds9 and I guess the packager didn't want to patch the setup.py in pyds9 to use the external xpa shared library.

On Ubuntu I see that there are separate python-pyds9, xpa-tools, libxpa1 and libxpa-dev packages.

Probably having XPA as a git submodule in pyds9 would be a good idea, but any change in the distribution / build system always has the risk to cause big trouble ... so I suggest we focus again on the main pyds9 vs pysao question and if anyone thinks the change to a git submodule for XPA should be made they open an independent issue.

ericmandel commented 11 years ago

so I suggest we focus again on the main pyds9 vs pysao question and if anyone thinks the change to a git >submodule for XPA should be made they open an independent issue.

I agree with this sentiment.

sosey commented 11 years ago

I just started looking into coding imexamine type functionality in python ( as in today ) and found this discussion. I've been looking at pysao/ds9 and soon Ginga but still have a bit to come up to speed. In the recent past I've looked at JJ's astronomy plotting stuff as well. I'd like to keep up with this conversation and any hangouts that happen if that's okay? - Megan

olebole commented 9 years ago

Hi all,

is there any new development here?

ericmandel commented 9 years ago

Not really, @olebole, @cdeil, @eteq, @timstaley, @sosey. But as part of the JS9 project we recently released pyjs9, which might serve as an example of the direction in which pyds9 could go. This module supports a low-level send routine (akin to pyds9's get/set routines) and two high-level APIs:

  1. a formal API (for programming) that returns objects containing typed arguments: SetColormap(cmap, [contrast, bias]) obj = GetColormap() where the return object contains colormap, contrast, bias properties
  2. an informal API that is easier to type and returns strings (as does pyds9): cmap(cmap, [contrast, bias]) "colormap contrast bias" = cmap()

Each API routine contains its own documentation, etc. See source code at:

https://github.com/ericmandel/pyjs9/blob/master/pyjs9/__init__.py

DS9 has many more public access routines than does JS9, and writing a separate routine for each one (packaging up the get or set command, and unpacking the result into an object) would be a lot of tedious work. And the, on top of those routine would come some better region manipulation routines ...

Give the amount of tedium and how busy everyone is, this probably would be a good Google Summer of Code project ... thought it's too late for this year.

sosey commented 9 years ago

I ended up going a mixed route for imexam. I used part of the cython xpa wrapper code that JJ had in pysao to wrap communication with ds9. I also wrapped a subset of ds9 functions which I used regularly, or seemed like most people used on a regular basis. The code is set up with a main access class for control which contains the viewer class code and imexamine functionality. Right now you can use either DS9 or Ginga (with @ejeschke recent awesome help) for viewing or use can use the imexamine functions separately without a display. I'm pushing the ginga updates today hopefully, but the current docs with examples and the api are online here: http://imexam.readthedocs.org/imexam/index.html

Imexam was meant to be used as a quicklook examination tool and I was planning on keeping it rather light so that it's really flexible for people to use, not as a fully functioned interface to DS9.

cdeil commented 9 years ago

In #14 @montefra adds Python 3 support to pyds9. As far as I can see pysao is non-active (last commit from 2013).

IMO it's important that one package gets buy-in and a small group of developers forms, and ideally it becomes a BSD-licensed Astropy-affiliated package.

So e.g. @sosey, but really everybody here: Are there any significant pros / cons of the pysao cython XPA interface versus the pyds9 ctypes interface? Would you be willing to use or contribute to pyds9 or do you absolutely need / want Cython?

@ericmandel - Concerning the high-level API: As far as I can see there is no pyds9 release on PyPI, on the other hand it's used by a lot of people. What's your opinion on backwards-compatibility? Maybe an 0.1 release should be made before we start improving the API (which will very likely always break someone's script, as soon as we change anything), but once 0.1 is out, any and all API improvements are welcome?

ericmandel commented 9 years ago

So e.g. @sosey, but really everybody here: Are there any significant pros / cons of the pysao cython XPA interface versus the pyds9 ctypes interface? Would you be willing to use or contribute to pyds9 or do you absolutely need / want Cython?

I should emphasize that I have no particular allegiance to ctypes. I had to pick an interface six years ago and it wasn't clear which way to go. If the community has decided that Cython is the preferred interface, we should be looking to convert at some point ... available time being the main problem.

ericmandel commented 9 years ago

What's your opinion on backwards-compatibility?

I'm a big fan of backward compatibility. But in this case, I see the current get() and set() methods as still-valuable primitives that would underlie a more sophisticated API. The point is that you eventually have to call either get() or set() to talk to DS9. So a higher-level API would take a list of useful arguments, package them up into the XPA paramlist and/or data buf, send the command(s) via XPA to DS9, and then unpack the results in a useful form for return to the user, e.g.:

SetColormap(cmap, [contrast, bias])

eventually has to call the equivalent of:

xpaset -p ds9 cmap [cmap]
xpaset -p ds9 cmap value [contrast] [bias]

depending on the arguments passed.

ericmandel commented 9 years ago

but once 0.1 is out, any and all API improvements are welcome?

Not sure about "any and all". I'd prefer a logically unified API. My initial view is that each DS9 access point can be mapped to a Python API call with a variable number of args, and that it would be a good thing to keep the Python API parallel to those access points. But on the other hand, the region issue will come up soon, since it will be important to send and receive regions in more reasonable format (see GetRegions in pyjs9) ... so a strict parallel to existing access points is not quite how it will end up.

olebole commented 9 years ago

@cdeil @ericmandel Since the original pyds9 already has version 1.7, please don't use 0.1; this will confuse the users. Maybe stay with 1.X with (roughly) the same API, and 2.X with an improved API?

ericmandel commented 9 years ago

Agreed. I see there are no tags in pyds9, so perhaps @montefra would want to create a 1.8 tag once the PR is merged.

montefra commented 9 years ago

@ericmandel I would tag it 1.7 as in the setup.py file. I think that we should do a few things: set up test #15, reorganize the package in a more pythonic way (I'll open an issue soon), think about the documenation. Then I would tag 1.8 and go to PyPI

ericmandel commented 9 years ago

That's fine with me.

cdeil commented 9 years ago

@montefra – For documentation Sphinx and readthedocs are used by most packages and IMO nice.

Just as an example ... this is the last package where I set that up:

Given that this is such a small package, I don't think we need to use astropy-helpers ... it's not that hard to integrate the test runner and Sphinx build in setup.py so that it works nicely.

I'd be happy to help add this for this package too, but one step at a time, getting some tests set up #15 should be the first step.

@montefra – Of course I'm super-happy if you do this stuff, but let me know if you want me to do something or review some PR.

cdeil commented 9 years ago

Actually yes, I misspoke, getting a release on PyPI so make pip install pyds9 works first, even before adding tests, makes sense. :+1:

montefra commented 9 years ago

@cdeil I know and use sphinx frequently for different projects I'm working on. Pure sphinx should be enough.

If you can setup #15 would be great. Once I'll merge #14 I'll try to figure out if and how I can enable travis (I want to do since a while, so I'll take the opportunity)

pip install pyds9 works first

It already works both from local directory and from git+https... It worked already when I started the PR #14.

sosey commented 9 years ago

Sorry I haven't had a chance to respond until now.

Personally I prefer the Cython interface to the XPA - C code, but I also have a bit of C experience. It's probably true that someone with more Python experience would feel more comfortable using the ctypes interface. There are cases where one or the other could be faster, for either maintenance or actual code speed. ctypes is often chosen for compatability and cython often for speed. I think if you wanted to work on extending the functionality of the XPA in the future Cython would probably give you more options than ctypes (maybe for dealing with larger datasets, explicit typing etc.)

I chose to use Cython for the imexam module, to wrap the xpa set() and get(). There's some overhead in terms of making the xpa class in the python code, but perhaps less overhead than you would get making all the functional calls through ctypes. For imexam, I then went on to create more convenient python wrappers to the other functions that DS9 provides, like panto, rotate and regions, so that the user didn't have to think hard about the format the XPA was expecting and could store information in python structures. These wrapper functions then just call the set() and get() directly. The set/get are also there for the user to decide to call them explicitly from imexam if they want to extend things themselves..similar to what @ericmandel mentions above. It would be nice to have a more flushed out pyregion module though..

For imexam, I'm getting around the byte/string return I saw mentioned because I found it faster to deal with the fits/byte arrays of images separately, meaning imexam keeps track of what's being looked at, but uses numpy for interfacing with the binary data, quicker to keep that array in memory and perform analysis then requesting data from DS9 all the time.

It would be nice to have a common interface for people. I think there's decent overlap of code between pyds9 and imexam, in terms of calling the XPA. Since I've already done the work for both the xpa and the higher wrappers, I'd prefer to keep with the cython interface, adding support for unicode/bytestrings would probably be minimal work on my end, but currently not needed in the code I use.

Imexam is my "in free time" project, it's already possible to use both the ds9 interface and the image examination functions as separate libraries from imexam with no added effort, so I often do that even when I just want to display a current fits image or numpy array to ds9 for visual inspection.

This is a little beyond the scope of the current discussion, but the thing I see that's missing that I'm not sure you can really solve right now with the DS9/XPA interface to python is active event monitoring and information passing. You have to ask the XPA for everything. Is there a current way to pass event information directly from the xpa/c-api that I'm missing? I played a little bit with merging python event monitoring of the mouse on the DS9 window, but then just went back to polling the xpa for the cursor location. If there's a way to open the ds9 event handler to the xpa, that might speak for using the cython interface. (sidebar, I'm not an expert on graphical interface and event handling...)

ericmandel commented 9 years ago

It would be nice to have a common interface for people.

Or at least a common philosophy about the user interface/structure of the two APIs ... and the "natural" agreement that everything is built on a low-level get/set is a really good start. The ctypes/Cython question is probably only important if we want to unify the two modules into one, but not if we simply want to have two nicely compatible interfaces.

Is there a current way to pass event information directly from the xpa/c-api that I'm missing?

No, I think you have to ask DS9 for the information. XPA has other capabilities (which could be added to DS9), but I don't recommend further proliferation of XPA in that way: it's seen its day. JS9 will be able to support active monitoring now that we can add socket.io to pyjs9, but that is a different story.

eteq commented 9 years ago

@montefra - just to clarify what @cdeil was saying about astropy-helpers and astroplan: they use the astropy affiliated package template (https://github.com/astropy/package-template). That's designed to basically make it much simpler for you to set up travis, py.test, and some sphinx extensions that make it easier to write "self-documenting" code (i.e., it automatically creates API docs from python docstrings in a reasonably sane way and prettifies them to look good with sphinx). So it may be worthwhile to use that for the ease of travis and the unit testing, even if you have familiarity with setting up sphinx on its own.

montefra commented 9 years ago

@eteq : Thanks for the link. I'll give a look at it later on. I have experience with sphinx, pytest, nose. But if can get something to make the work for me, I'm happier :D

eteq commented 9 years ago

@montefra - Ok cool. And as a bonus, it makes for a (at least slightly) more consistent developer and user experience between astropy and the affiliated packages, so users/developers have a bit less to learn if they want to contribute.

eteq commented 9 years ago

It would be nice to have a common interface for people.

Or at least a common philosophy about the user interface/structure of the two APIs ... and the "natural" > agreement that everything is built on a low-level get/set is a really good start.

Absolutely this is a good start, but I think I agree with @sosey that actually having the same implementation here may be important. I've encountered quirks about how XPA interacts with python in pyds9 that I had to work around (not necessarily a bug that required reporting, just quirks in various unusual FITS files), and in at least one case a different implementation broke the workarounds. So for situations like that I don't have a particular strong opinion how it's done, but I just want the packages to do the same thing. But I admit I'm not a typical "user"...

@sosey, do you expect it would be a lot of work to try something like either switching pyds9 to a Cython-based approach or adapting imexam to use pyds9? Or does it seem like e.g. you could just swap imexam's interface to XPA get/set with pyds9?

ericmandel commented 9 years ago

actually having the same implementation here may be important.

I'd like to understand this better. When retrieving FITS files from DS9 using any interface (pyds9, xpaget from the command line, the XPAGet() C routine, etc), the results had better be byte-for-byte identical with one another ... and with what DS9 is sending out. Otherwise, there's a mistake somewhere.

That said, I'll re-emphasize that I chose ctypes semi-arbitrarily based on my limited understanding of what was best practice back in 2010, and this might have changed or evolved. So I am open to changing pyds9 as needed.

eteq commented 9 years ago

The particular issue I had in mind here was not at the DS9 level, but rather with how the byte-stream got re-interpreted into various byte/string/numpy array, etc. constructs. If I recall right, it was a situation where it was a FITS file that had unusual endianness, and one of the packages made a different choice about when to switch to my system's native endianness (I think pyds9 does it as part of a separate array conversion step, while the other one must have done it actually as a part of get or something).

To be clear this has nothing to do per se with the cython/ctypes question. I'm just saying that there are nearly always quirky arbitrary choices in an implementation, so using just one low-level implementation means users only have to worry about that one implementation.

ericmandel commented 9 years ago

Yes, I recall that you fixed a bug in pyds9 (thanks again) to make an endian issue work properly.

The common low-level implementation you are talking about probably has to include get/set as well as routines to read/write FITS and arrays to/from numpy and astropy/pyfits, i.e.:

sosey commented 9 years ago

@eteq I strongly prefer to keep the imexam module using the cython interface to the xpa, especially for the particular application imexam is meant for, rather quick image examination combined with plotting. I found that the cython calls provide less overhead than ctypes while still giving me direct access to the xpa. If I had to rewrite the interface to pull in pyds9 and deal with those overheads I think it would slow down the process and user experience because I'm making lots of rapid calls into the xpa library.

I think there are 2 particular regimes in which people are probably using DS9 for their work:

1) They are using it as an atomic image analysis tool. They want to display their images, and use the DS9 analysis functions for examining the pixels, making plots, playing with scales, etc and at the same time, they want access to all the input and output from DS9 through a Python terminal session.

:: For this case, I think the current ctypes interface is fine, people probably aren't looking for speed and they don't have lots of their own code with which to interface.

2) They are using DS9 purely as a display for their image, it's image codecs are written in C and very fast at display and manipulation which is a big bonus, however, they would like to use their own image analysis and plotting functions and merely push the results to the display or pull pointer/mouse locations.

:: For this case, I think the cython interface works better. The calls are quicker, and really you're starting with all your data from inside your python session, using local python tools or functions you've created to do the analysis, ds9 is merely the display window. You don't have to ask the XPA for numpy or binary arrays, you just push them.

In order to accommodate both applications I think the cython interface would be ideal. It wouldn't be much work to switch pyds9 over to xpa. Many of the functions I see in pysd9 are similar. Much of the management that has to be done is implemented the same way in pyds9 and imexam. If you wanted to switch pyds9 to using cython, I'd give the changeout a go in imexam and support updating pyds9 along with @montefra. I think I'm only using the set() and get() right now, so I'd have to add the cython calls into the other access points. I have much of the user friendly functions already wrapped, and they just call the set/gets so no real updates needed there, we could just add them in to the new package.

ericmandel commented 9 years ago

@sosey @eteq It sounds like the next step is for "someone" (presumably meaning me) to try switching out ctypes for Cython in pyds9. I'll put this on my TODO list.

So far as I can tell, making this switch will not solve the issue raised by @eteq:

packages made a different choice about when to switch to my system's native endianness

but we'll deal with that as we go along.

@montefra I'll work in python 2.7 and assume you will look at p3 issues at some point, if necessary.

montefra commented 9 years ago

@ericmandel: ok.

Can you open an issue about the switch?

eteq commented 9 years ago

That seems like a good plan - if it turns out there are genuinely different use cases for the implementations (i.e., it's hard to switch pyds9 to something that could be compatible with imexam for whatever reason), then practicality beats purity.