ericmandel / pyds9

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

astropy and numpy are now dependences, pyfits can be used. #56

Closed montefra closed 6 years ago

montefra commented 6 years ago
montefra commented 6 years ago

@ericmandel : I have began to add tests.

As far as you know, is there any way to run ds9 without the GUI?

ericmandel commented 6 years ago

As far as you know, is there any way to run ds9 without the GUI?

@montefra You need an X server, but you can use the virtual Xvfb. See:

http://ds9.si.edu/doc/faq.html

for details. @wjoye will have more recent experience with this than I.

montefra commented 6 years ago

Thank you for the pointer. I'll see if I can use this: it might be enough. For now I just want to check that I can send and receive data.

montefra commented 6 years ago

there is a pytest pluging for Xvfb. I'll give a try in the next few days.

ericmandel commented 6 years ago

That would be pretty convenient!

montefra commented 6 years ago

@ericmandel : good news! pytest-xvfb works and I can test ds9. I have added tests for {set,get}_fits. I will add similar tests for {set,get}_pyfits and then I will call this issue closed.

I'll add inline comments as review to clarify the points. Can you please go through them and let me know if something doesn't make sense?

ericmandel commented 6 years ago

@montefra Better than good news, this is great news and great work. The whole Python community owes you a debt ...

The changes looks fine and make sense, although I hasten to admit that if there are subtle details, they may be a bit beyond me, my Python knowledge having leaked away due to non-use.

Again, it's really good work.

montefra commented 6 years ago

The changes looks fine and make sense, although I hasten to admit that if there are subtle details, they may be a bit beyond me, my Python knowledge having leaked away due to non-use.

The main thing is to check that I am not doing something stupid with the way I communicate with ds9, in particular when using xpaget.

Again, it's really good work.

Thank you :blush:

montefra commented 6 years ago

thank you for checking. Sometimes English slips ...

On the {get,set}_pyfits: I tried to keep its behaviour as similar as possible to the old one. If pyfits is not installed, warn the user to use {get,set}_fits. Otherwise {get,set}_pyfits gets or returns a pyfits.HDUList instance.

montefra commented 6 years ago

If you don't have any request or comment, I might merge this PR

ericmandel commented 6 years ago

It looks fine, I'd merge it and take the rest of the day off!

montefra commented 6 years ago

@ericmandel : do you ha ve preferences on how to do the merging?

The merge button shows three options:

  1. Merge pull request: I guess that it does a merge or a fast-forward depending on the status
  2. Squash and merge: I don't like the idea of squashing
  3. Rebase and merge: first make sure that the changes are on top of the master and then do a fast-forward.

I think that for this case 1. and 3. are the same

ericmandel commented 6 years ago

I generally just merge. Both rebasing and squashing can hide the actual flow of development and, in general, I would prefer to have the commits match that flow.

montefra commented 6 years ago

@ericmandel : fine with me.