search

ericmandel / pyds9

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

pyds9.ds9_openlist: too many instances #61

Closed montefra closed 6 years ago

montefra commented 6 years ago

Actual Behavior

If there are multiple instances of ds9 opened without an explicit title, pyds9.ds9_openlist fails with ValueError: too many ds9 instances for target: DS9:ds9.

pyds9.ds9_tragets correctly returns two elements.

Expected Behavior

pyds9.ds9_openlist returns one instance of pyds9.DS9 for each element of pyds9.ds9_tragets

Steps to Reproduce the Problem

  1. Open two files with ds9

      ~> ds9 file1.fits
  ~> ds9 file2.fits

  2. Open python/ipython

      >>> import pyds9
  >>> pyds9.ds9_targets()
  ['DS9:ds9 /tmp/.xpa/DS9_ds9.15900', 'DS9:ds9 /tmp/.xpa/DS9_ds9.24658']
  pyds9.ds9_openlist()                                                            
  More than one ds9 is running for target DS9:ds9:   
    DS9:ds9 /tmp/.xpa/DS9_ds9.15900                                                  
    DS9:ds9 /tmp/.xpa/DS9_ds9.24658                                                     
  Use a specific name or id to construct a DS9 object, e.g.:
    d = DS9('ds9')                                                           
    d = DS9('DS9:ds9')                                                       
    d = DS9('/tmp/.xpa/DS9_ds9.15900')                
  The 'local file' id (3rd example) will always be unique.  

  ---------------------------------------------------------------------------             
  ValueError                                Traceback (most recent call last)
  <ipython-input-4-eeaf3a1d4a30> in <module>()                                       
  ----> 1 pyds9.ds9_openlist()                                                          

  pyds9/pyds9/pyds9.py in ds9_openlist(target, n)
      361         ds9list = []        
      362         for item in bytes_to_string(tlist):                           
  --> 363             ds9list.append(DS9(item.split()[0]))
      364         return ds9list                                             
      365                                                                    

  pyds9/pyds9/pyds9.py in __init__(self, target, start, wait, verify)
      493             print("  d = DS9('%s')" % a[1])
      494             print("The '%s' id (3rd example) will always be unique.\n" % s)     
  --> 495             raise ValueError('too many ds9 instances for target: %s' % target)
      496         else:                                                              
      497             a = tlist[0].split()

  ValueError: too many ds9 instances for target: DS9:ds9

Specifications

I will investigate the issue further

ericmandel commented 6 years ago

Yeah ... this probably is because I did not know what pyds9.DS9() ought to return if there are multiple instances with the same name. Right now, it's an error if pyds9.Ds9() is called and there are multiple instances satisfying the target:

d = pyds9.DS9("ds9")

And I think this is correct behavior: if you only want to open one instance, and there are two, you should be made to choose which one to open.

One could argue that pyds9.ds9_openlist() should happily open multiple instances with the same name, but the problem is that it uses pyds9.DS9(), which does not allow for multiple instances. I suppose we can pass a "returnMulti" or "returnArray" flag to pyds9.DS9() (default is false) to return an array of all opened instances -- in this case, pyds9.ds9_openlist() is just a very thin later setting the flag to true.

But I assume the Python community has already wrestled with this problem, so probably there is a canonical technique!

montefra commented 6 years ago

Right now, it's an error if pyds9.Ds9() is called and there are multiple instances satisfying the target:

d = pyds9.DS9("ds9")

And I think this is correct behavior: if you only want to open one instance, and there are two, you should be made to choose which one to open.

I agree that this is the correct behaviour

I suppose we can pass a "returnMulti" or "returnArray" flag to pyds9.DS9() (default is false) to return an array of all opened instances

I disagree here: DS9 is a class representing a ds9 instance. If it cannot decide which one it should crash

I think that the solution is quite simple: pyds9.ds9_targets() returns a list of strings like:

['DS9:ds9 /tmp/.xpa/DS9_ds9.15900', 'DS9:ds9 /tmp/.xpa/DS9_ds9.24658']

or

['DS9:foo1 838e29d4:42873', 'DS9:foo2 838e29d4:35739']

each string is composed by a name and a path/address/..., e.g. /tmp/.xpa/DS9_ds9.15900 and 838e29d4:42873. If they are unique, we can create the DS9 instances using the path, instead of the name. It should be enough to replace this line

https://github.com/ericmandel/pyds9/blob/3e49b2a0c4ff1a162c9cc936177a5442191263fc/pyds9/pyds9.py#L363

with

ds9list.append(DS9(item.split()[1]))

Do you think that this the solution to the problem?

ericmandel commented 6 years ago

I think that will work for multiple instances -- very nice, actually -- but could be confusing for one instance, since the target is saved:

 self.__dict__['target'] = target

In the single instance case, this change will result in saved target having an unexpected name. Perhaps you could split between [0] for single instances and [1] for multiple instances?

montefra commented 6 years ago

but could be confusing for one instance

do you mean that you would expect ds9.target to be:

Perhaps you could split between [0] for single instances and [1] for multiple instances?

That's a way. I can also implement a way to have the nice name in target if only one instance with that name is present and the path/address otherwise. Something along these lines. If

>>> pyds9.ds9_targets()
['DS9:ds9 /tmp/.xpa/DS9_ds9.15900', 'DS9:ds9 /tmp/.xpa/DS9_ds9.24658', 'DS9:name /tmp/.xpa/DS9_ds9.25042']

then pyds9.ds9_openlist() returns 3 DS9 instances with target attribute:

Would you prefer this latter option? This is a few more lines of code, but it's not more complicated

ericmandel commented 6 years ago

'DS9:ds9', when only one instance is present '/tmp/.xpa/DS9_ds9.15900' when multiple instances are present?

Precisely.

I'm not clear about the implementation alternatives (i.e. where you split one instance processing and multiple instance processing), but the output you show/describe for both ds9_targets() and ds9_openlist() is exactly what I think we need. So however you want to get there is fine ...

montefra commented 6 years ago

62 resolveds the issue