Merge: SAOImagesDS9.jl + DS9ui.jl

[icweaver]

Brought up in: JuliaRegistries/General#133313

Companion to: astrozot/DS9ui.jl#3

[astrozot]

Dear all, I made a fork of SAOImageDS9.jl and I am working at the merging. All changes I make will be reported in a CHANGELOG.md file.

I anticipate here that I would like to make one particular change that allows us to have a more general interface. Right now the code saves the address of the current access point. I believe it is better to save the full access point, as this allow us to use an optional first parameter in all calls referring to the access point. In other words, one could do

SAOImage.set([accesspoint], command)

to send the command either to the default or to a specific access point. That would not be possible with an address, as it is a string and could be confused with the command.

[emmt]

@astrozot As I wrote in the discussion about the PR for DS9ui, I would be happy to accept contributions and to fuse your interface into SAOImageDS9 to reflect your needs.

Before you (@astrozot) spend to much time on a PR, we may perhaps iterate a bit on what you miss and how do you want to have it implemented in SAOImageDS9.

As a guideline, one my objectives was to make the interaction with DS9 intelligible to a Julia user. The syntax of DS9 XPA commands is quite involved and not fully correctly documented (some examples just do not work). This is why SAOImageDS9 provides a command like DS9.draw(something; options...) which manage to draw something (an images, somes points, shapes, etc..) in DS9 based on the type of something (thanks to Julia's dispatching). The TwoDimensional package and other officila Julia packages define a number of shapes that could be used as something (using the extension mechanism to not strongly depend on too many external packages).

The principle to send a command to a default access point (if not specified) is already there in SAOImageDS9 although perhaps not correctly implemented as suggests the above comment. It would be more than happy to fix this.

There are some other functionalities of DS9ui already implemented in SAOImageDS9 like SAOImageDS9.select which is equivalent to DS9ui.ds9select.

I admit documentation of SAOImageDS9 is largely imperfect. I would very much like taking the opportunity of this discussion to improve it. I think this could be largely done by providing examples.

There are some design differences between DS9ui and SAOImageDS9. I have seen that in DS9ui all functions are prefixed by ds9, e.g. ds9image, which make them suitable to export without conflicts. in SAOImageDS9, I took a different option: the package exports a symbol DS9 which is an alias to SAOImageDS9, then methods are simpley prefixed by DS9., e.g. DS9.select to avoid conflicts while making clear to which module the method belongs to.

[astrozot]

Dear @emmt ,

I worked already a little at the merge. You can have a look my efforts in the fork

https://github.com/astrozot/SAOImageDS9.jl

The CHANGELOG.md file summarizes all changes I made. As you will see, I tried as much as possible to adhere to the Julia Style Guide and to the Blue Style.

I like the idea of having a higher level interface, but as you say this is quite involved as one has to fight with the very old and somewhat inconsistent syntax used in the DS9 XPA commands. As a start, I (re)implemented a few methods towards this goal: a couple to open and close the DS9 application, and one get method that automatically converts the output, without any need to specify the type and dimension (works quite well with both scalar and vector outputs).

For the rest, probably a reasonable approach is to start implementing what we believe are the most needed commands, and incrementally add stuff. Personally, I would like to have a way to extract bit masks from DS9, as this is something I often need to do, and here the nice TwoDimensional package could be very handy.

For the export names, either way is OK with me; I saw you export DS9 as an alias for SAOImageDS9. However, I never saw this done in other packages and I am wandering if one could not just use

import SAOImageDS9 as DS9

to obtain the same effect (this should be OK because you do not export any other symbol, right now). Having the ds9 prefix has I do is perhaps just slightly more handy to type, but really this is such a minor issue and we might want to retain backward compatibility here.

For the rest I am of course open to any discussion. Perhaps you can start trying my fork and let me know what you think.

[emmt]

I had a look at your changes and have some thoughts, not all related to your new functions but also to the current API of SAOImageDS9 which has some defects that become evident with your contribution. So I want to push the discussion a little further because I think that it is time to improve on this API to facilitate user experience and make extending the package a bit easier. In this discussion I'd like to distinguish the functionalities that you want to add and the API to smoothly implement these functionalities but also the existing ones and the future ones.

1. Saving the full APT address of the running DS9 is indeed a better idea. I would however have a slightly different API than what you implement for getting/setting this but that's a minor point. I think that the way the connection to DS9 is managed by all public methods (the ds9... ones) should be changed. Based on the fact that, most of the time, users want to direct commands to the default DS9, it seems better to have apt an optional keyword rather than an optional leading argument. This would also simplifies the number of methods to implement.
2. Rethinking about suffixes, I think your solution is better in fact. For example, in XPA.jl and SAOImageDS9.jl there are two functions get and set that are specific to these packages and yet different from the Base ones. When reading or writting code inside one of these package there are some potential confusions. Having named them xpaget, xpaset, ds9get, and ds9set would have been a better idea for the users but also for the maintainers of the code. From inside each package, we can have aliases but changing these names will constitute a breaking change but it is time for a new version, right? So my proposition is to have public (and exported methods) prefixed by ds9 as you start to do and have, in SAOImageDS9.jl, aliases like const xpaget = XPA.get and const xpaset = XPA.set to XPA.jl methods.
3. I like your idea that SAOImageDS9.get can automatically guess the type of the result. This is a nice feauture for the REPL but this is not type-stable and having the expected type of the result optionally specified should remain for scripts and for implementing other methods.
4. There is already an SAOImageDS9.select function in SAOImageDS9.jl with a different purpose than your ds9select: mine is to get the cursor (XPA command iexam), yours is to select the DS9 "window" (I think "application" is ;ore appropiate). The two methods have to be rethought. At least one has to be renamed because they have different meaning. I think that yours is more related to the connect one. I like your idea to have some interaction with the user if there are several running DS9, so my suggestion would be to merge your ds9select and SAOImageDS9.connect in a single ds9connect method and rename SAOImageDS9.select to something like ds9cursor. Your ds9 method will probably be used only a few times per session and I would rename it ds9launch or, perhaps better since it changes the default accesspoint, make it a special case of the ds9connect method.
5. More generally, we have to think about some "logic" to name public methods otherwise the users will be confused. A possibility is to use the same name as the XPA command implemented by DS9 but that would represent a lot of names to remember and, as you also pointed, these command are not well documented and their syntaxes are a bit weird. So having something more understandable and more "Julian" (i.e. with the precise meaning of the command inferred from the type of the arguments) has my favor. BTW this is the kind of interface I tried to adopt for SAOImageDS9.draw. Another possibility is to get our inspiration from other interfaces to DS9 like those in Python.
   Now the question is how shall we proceed? My proposition is to review the functionalities we would like to have and how users will access them. It is not a question of listing all possibilities but rather to have some pattern that has its logic and that can be easily extended without changing this logic. For a concrete illustration, we can start a "devel" branch into which I could merge your changes then modify according to the new API.

To be more specific, here are what I would like to have after merging your changes and applying the above ideas:

1. Having the full access point stored in a global variable (more or less what you did). This access point is the default one. Methods have an apt keyword to choose another one (it is not an optional leading argument). Public method ds9connect to deal with the connection to DS9: set the default access point (with possible user interaction if there are several running DS9).
2. Public ds9launch method to launch DS9 and make it the default access point.
3. Public ds9get and ds9set methods (instead of SAOImageDS9.get and SAOImageDS9.set) with ds9get able to infer the type of the result.
4. Rename SAOImageDS9.draw as ds9draw and SAOImageDS9.select as ds9cursor.
5. Of course keep your ds9wcs and ds9getregions. Perhaps rename ds9getregions as ds9regions unless there is a planned ds9setregions.

Let me known what you think...

[astrozot]

Hi, thank you for taking the time to check the code. I think we essentially agree on everything, but let me just reply to your individual points for clarity.

Saving the full APT address of the running DS9 is indeed a better idea. I would however have a slightly different API than what you implement for getting/setting this but that's a minor point. I think that the way the connection to DS9 is managed by all public methods (the ds9... ones) should be changed. Based on the fact that, most of the time, users want to direct commands to the default DS9, it seems better to have apt an optional keyword rather than an optional leading argument. This would also simplifies the number of methods to implement.

I understand your view, but in my opinion it would be better to keep an optional first argument. In fact, this complies with the general use of this kind of arguments in Julia (see the ordering of arguments in the Style Guide). The way I see the default access point is similar to a IO argument which can be present or missing in functions such as print or show; the same approach, actually, is used also by packages such as Makie, where all plotting commands have an optional first argument (the Axis).

Regarding the simplifications of methods to implements, if you look at my code you will see that in reality there is no much to do: one just need to add a single additional method, and make sure that all other methods have a first XPA.AccessPoint argument, so there is no real extra effort.

Rethinking about suffixes, I think your solution is better in fact. For example, in XPA.jl and SAOImageDS9.jl there are two functions get and set that are specific to these packages and yet different from the Base ones. When reading or writting code inside one of these package there are some potential confusions. Having named them xpaget, xpaset, ds9get, and ds9set would have been a better idea for the users but also for the maintainers of the code. From inside each package, we can have aliases but changing these names will constitute a breaking change but it is time for a new version, right? So my proposition is to have public (and exported methods) prefixed by ds9 as you start to do and have, in SAOImageDS9.jl, aliases like const xpaget = XPA.get and const xpaset = XPA.set to XPA.jl methods.

I fully agree, and this is the time for a new major version which will include breaking changes. The only (minor) question is whether we want to define xpaset and like, or just let the user decide what to do with XPA. Most people will not use the XPA.set and XPA.get commands, so perhaps there is no need to create extra definitions. But if you feel this would be useful to have, let us define them.

I like your idea that SAOImageDS9.get can automatically guess the type of the result. This is a nice feauture for the REPL but this is not type-stable and having the expected type of the result optionally specified should remain for scripts and for implementing other methods.

Yes, I fully agree, we should keep the original code as well which has the advantage of being type stable. I believe my fork keeps the original definitions, so if there is a type in the initial arguments this is used (I hope I did not delete anything here).

There is already an SAOImageDS9.select function in SAOImageDS9.jl with a different purpose than your ds9select: mine is to get the cursor (XPA command iexam), yours is to select the DS9 "window" (I think "application" is ;ore appropiate). The two methods have to be rethought. At least one has to be renamed because they have different meaning. I think that yours is more related to the connect one. I like your idea to have some interaction with the user if there are several running DS9, so my suggestion would be to merge your ds9select and SAOImageDS9.connect in a single ds9connect method and rename SAOImageDS9.select to something like ds9cursor. Your ds9 method will probably be used only a few times per session and I would rename it ds9launch or, perhaps better since it changes the default accesspoint, make it a special case of the ds9connect method.

I agree again, we need to rethink the names. Your proposals sound very reasonable: so we would have

• ds9launch (or perhaps even ds9open?) to launch/open the DS9 application; here we should decide how to deal with multiple launches (right now the title is the process ID of the launching Julia REPL, but perhaps we want to allow multiple applications?)
• ds9exit (or ds9close) to exit/close it
• Perhaps something like ds9connect or ds9select (or ds9setdefault or whatever you like) to select a specific access point
• ds9set and ds9get as relatively low-level interfaces
• ds9cursor to get a position
• ds9draw to draw stuff
• ds9getregions to obtain the regions (I do not have a ds9setregions implemented yet, but this will be done almost certainly, so I would keep a more general name here)
• all the rest...

Let me know your opinion. As soon as we settle these points I think we can in a relatively short time make a new release. We can then iterate for further functionalities without breaking anything.

[astrozot]

I forgot one point. I had a look at the code in Makie to obtain the current figure, as I thought we could get ideas to better implement the current access point. The code is the following

const CURRENT_FIGURE = Ref{Union{Nothing, Figure}}(nothing)
const CURRENT_FIGURE_LOCK = Base.ReentrantLock()

"""
    current_figure()

Returns the current active figure (or the last figure created).
Returns `nothing` if there is no current active figure.
"""
current_figure() = lock(()-> CURRENT_FIGURE[], CURRENT_FIGURE_LOCK)

"""
    current_figure!(fig)

Set `fig` as the current active figure.
"""
current_figure!(fig) = lock(() -> (CURRENT_FIGURE[] = fig), CURRENT_FIGURE_LOCK)

As you can see, it always acquire a lock before reading or writing the CURRENT_FIGURE Ref. I do not have a clear view here: perhaps this is an overkill for our purposes, or perhaps it is a good idea to write the code along these lines?

[emmt]

For the first argument to specifiy another server than the default, I agree that this is similar to optional io::IO elsewhere but if you look in the code, you'll see that many functions were quickly written and do implement this optional first argument. They will have to be fixed in that respect if we keep the optional first argument for the access point. Otherwise I agree that this makes sense so let's say that we keep that feature. I realize that the fact that this argument is a structure XPA.AccessPoint is just to avoid ambiguities, in fact only the address part is used by XPA.get and XPA.set. I may simplify this a bit to allow for specifying the "address" more directly (i.e. without calling the XPA nameserver which has some deficiencies, see below) when you know it. It is the field XPA_METHOD in DS9 menu File -> XPA -> Information.

The lock (as in Makie) is only necessary if two requests cannot run in parallel without conflict. We have to check that but as far as each task has its own client connection this should be ok. BTW I realized that there were some misconceptions in XPA.jl notably regarding the sharing of ressources. In old Julia, it was sufficient to index the ressources by the thread ID but now that Julia tasks can migrate to another thread this is wrong. I am fixing that and also issues about the garbage collector. Looking in your code I realized that you use the environment variable XPA_METHOD to restrict the kind of connections. This is a good idea but it does not work to change this variable after having calling XPA.list() a first time (at least on Linux). To solve this I will probably directly call the xpaget xpans command (not the XPAGet function of the library) which is bundled in XPA_jll and hope this will be portable. So expect a new release of XPA.jl very soon and probably with xpaget, xpaset etc.

BTW, what is the machine you are using for your tests?

In the mean time, I also started to look in more details to your code and I have a few fixes. So my proposition is that I will merge your PR in the new https://github.com/JuliaAstro/SAOImageDS9.jl/tree/integrate-ds9ui branch I have just created and start working on this...

[astrozot]

Julia task migration has created a few headaches to me too in various situations...

I am using a Mac. On my computer I was unable to use any XPA_METHOD different from "local" (this is perhaps related to the firewall).

Please go ahead with the merge in the new branch.

[emmt]

I will not be availble until the middle of next week so I give you some news of my progresses.

1. I have merged @astrozot PR into a new integrate-ds9ui branch but only on my own copy of the repository. Indeed there were many conflicts to solve due to the fact that some recent SAOImage/DS9 commits were not part of the PR (no problem, I am not complaining). The result is highly non functional so I prefer not to push on GitHub yet. I also want to come to an agreement (at least with @astrozot, @abhro, and @icweaver) for how to connect/disconnect to DS9.
2. My priority was to solve pending problems (garbage collector, per-task connection instead of per-thread connection, etc.) in XPA.jl. I gave priority to XPA, because SAOImage/DS9 is based on it and depends on the evolution of XPA API.
3. Another reason to first fix XPA.jl is that some nice ideas of @astrozot (like interacting with the user selecting a single server) could be directly integrated in XPA.find and XPA.list. See the Unreleased changes in the top of XPA.jl/NEWS.md for more details.
4. The last reason was that the attempts of @astrozot to select a specific connection method in SAOImage/DS9 (like the unix socket which is more secure to which I totally agree) can only work if the XPA nameserver is not yet launched. This had to be fixed if possible in XPA.jl rather than in SAOImage/DS9 . This is done by this commit by calling the xpaget executable to retrieve the list of servers instead of using the C function XPAGet which is too limited.

I think that XPA.jl has now a much better API (simple and yet flexible thanks to predicates) to find the access-points of XPA servers and that should be beneficial to SAOImage/DS9. Before making a new release, the doc. of XPA.jl needs to be updated to reflect these changes (currently only the old behavior, now deprecated, is documented) and to solve JuliaAstro/XPA.jl#10.

[emmt]

Hello,

The XPA.jl API is now stable again (I think). The changes are listed in the Version 0.3.0 section of XPA.jl/NEWS.md. I am ready for a new minor (0.3.0 as said) release as the API has changed with (very few) incompatibilities. The server part is still a WIP. For exemple, I would like to have servers run in Julia tasks to avoid blocking Julia to handle clients requests, but this can wait the 0.4.0 release. Having XPA servers correctly done is also needed to implement unitary tests (only 15% of the code is covered). Anyway, the new possibilities and the bug fixes of XPA.jl make it suitable to extend and improve SAOImageDS9.jl.

One of the issues, I had to face was the number of potential ambiguities due to the flexibility of the arguments that can be taken by XPA.get but also by the now deprecated XPA.get_data (now you should use the data property of replies). Having the access-point of the server after optional type T and dimensions dims help a lot to reduce the ambiguities. However in current SAOImageDS9 API the access-point is before T and dims which is not only inconsistent with XPA.jl but also more problematic regarding ambiguities. Unles you (@astrozot, @icweaver, or anyone following this discussion) are strongly against this change, I will move the optional access-point in SAOImageDS9 following the API of XPA.jl. I do not think that this is a mjor problem for anyone as this argument is almost never explictely specified (the default access-point to DS9 is used).

[emmt]

I have merged @astrozot PR with my changes to make a new SAOImageDS9 API largely following the above discussion. Most changes are summarized here: #17.

I have not tested ds9getregions which may have to be changed a bit due to the changes in the API. Others exported methods seem to work (including ds9wcs).

Compared to the above discussion, there are few changes:

• ds9close or ds9exit is finally ds9quit because quit is the name of the corresponding XPA command.

• Although I first found that having ds9get able to apply a heuristic to convert the answer, I decided to not implement this feature for the following reasons:

1. I want to favor type-stable results which obviously could not be the case if ds9get parse the result in different possible ways with no means to infer which one in advance.
2. Parsing or scanning the answer of and XPA get request (interpreted as an ASCII string in that context) is a totally different thing than converting the binary answer data to some given Julia type (including String). The old SAOImageDS9.get was mixing the two kinds of operations which I found confusing (and which may be type-unstable). Since XPA.get is capable of doing the latter operation (converting the binary content), I decided that this should also be the case for ds9get. Since the first operation is really needed, I added a new ds9scan(T, args...) which concatentes args... into a single XPA get command, send it to the SAOImage/DS9 server, and eventually scan the answer data interpreted as an ASCII string to return something of type T. In ds9scan(T, args...), the result type T can be a scalar type like Int or String, a vector type like Vector{Int}, or a tuple type like Dims{3} or Tuple{String,VersionNumber} (to parse the result of the "version" command), etc.
3. So ds9scan is quite flexible but, for now, does not allow for a type-unstable result. For me this is not an issue on the contrary because, when sending a command, it is more secure to parse the command according to its expected format rather than rely on some heurist that cannot detect that whether there is something wrong in the answer. For answers to request like "fits size" which may return 2 or 3 (I guess) dimensions, there are simple solutions: use Dims{2} or Dims{3} for T if you know or want to impose the number of dimensions and use Vector{Int} otherwise.

Of course, let me know if you disagree.

Ownig to the number of changes, I think that it is better to wait for a few days before making a new major (0.3.0 or, why not, 1.0.0) release. I also have to update the doc.

[astrozot]

Hi @emmt, thank you for your work and for merging my PR. Regarding the XPA arguments, I believe we should try to be consistent with the standard Julia ordering of function arguments, as I mentioned already. The access point, to me, looks like an IO argument, and therefore should precede the type. I understand ambiguities can be quite annoying, but I believe there were no ambiguities in the PR I submitted and that the code was OK (I mean, not too ugly... :-). Did you find any ambiguity in my PR? If not, could one use the same technique in XPA.jl?