search

ejeschke / ginga

The Ginga astronomical FITS file viewer
BSD 3-Clause "New" or "Revised" License
121 stars 77 forks source link

Modify Crosshair to contain the Cuts plot #952

Closed ejeschke closed 3 years ago

ejeschke commented 3 years ago

This PR moves the "Cuts" plot and functionality from the Pick plugin to the Crosshair plugin.

Pick still has the "Quick Mode", which still works as the tool tips suggest, but simply does not activate a Cuts plot.

EDIT: Close #951

pllim commented 3 years ago

I don't have objection to this, as I don't use the Quick Mode in Pick myself. I wonder if @obi-wan76 uses it...

ejeschke commented 3 years ago

So I think this will help alleviate some confusion about the Cuts plot that is in Pick. This moves it to Crosshair. Crosshair also opens into a different workspace, and can work in concert with Pick.

The Cuts plot and Pick now work the way I had envisioned it, which is that the plot updates with a rough data profile as you mouse around the image and then for objects you want to examine more thoroughly you can press 'r' while the crosshair is on the object and it will invoke a Pick on that object.

obi-wan76 commented 3 years ago

@pllim yes, we use it. All this look good, thanks for the update!

pllim commented 3 years ago

Also, should we move Crosshair from "Utils" category in the "Operation" menu to "Analysis"?

ejeschke commented 3 years ago

Given that there is a new plugin_Crosshair.cfg file, it needs to be inserted into the documentation, not unlike how it is done in Pick. If you need help with this, I'll be happy to PR to this PR; please let me know.

@pllim, is this as simple as adding:

# Append module docstring with config doc for auto insert by Sphinx.
from ginga.util.toolbox import generate_cfg_example  # noqa
if __doc__ is not None:
    __doc__ += generate_cfg_example('plugin_Crosshair', package='ginga')

to the bottom of the Crosshair.py ?

ejeschke commented 3 years ago

Unlike Pick, Crosshair dialog appears on the left, not right. I was a little surprised. Why this change?

Because under the intent of the plugin, you want to be able to mouse around quickly to evaluate levels on various objects and then view the Pick results for ones that you select. It becomes a pain if you have to constantly be switching tabs to go back and forth. That is why it makes sense to move the "live" cuts plot to somewhere else. It wouldn't have to be Crosshair, but Crosshair had a very minimal UI, and this possibly makes it more useful, whilst also allowing a sort of synergy with Pick.

I had already been thinking about moving the plot, but when you said you didn't understand why it kept changing while the other plots were static (in #941 review), it confirmed that thinking. I think it will lessen confusion about it's use.

ejeschke commented 3 years ago

Also, should we move Crosshair from "Utils" category in the "Operation" menu to "Analysis"?

Ok, by me.

ejeschke commented 3 years ago

I wonder if it is easier to Photoshop the tab out than to take new screenshots... thought_balloon

Wouldn't hurt to make fresh screenshots...

pllim commented 3 years ago

Re: docstring -- yes, and the heading like this:

https://github.com/ejeschke/ginga/blob/52e4b7593fc3a98d57f47ebbcfb3aa67491209cd/ginga/rv/plugins/Pick.py#L306-L307

ejeschke commented 3 years ago

It is also unclear in the Crosshair doc that the profile plot is interactive.

In this case, the ginga plot is being used for performance mainly and not interactivity. The thing is, if you move the cursor to the plot to manipulate the plot, you've lost the plots you were looking at. I guess if "Drag only" is selected it could make sense to manipulate the plot...

.. but scrollbar didn't appear like your demo

I wasn't enclosing the viewer in a ScrolledView. Fixing that. I'll add a blurb in the plugin doc about the interactivity.

ejeschke commented 3 years ago

It is unclear in the Crosshair doc how red/yellow status threshold is really calculated. I have a feeling it is not simply based on the central pixel value under the cursor.

I'll add a blurb to the doc.

ejeschke commented 3 years ago

@pllim, thanks for a good review! I believe I have addressed all points in one way or another. When you have a chance take a quick look, esp. at the documentation that I added/updated and new Pick screenshots.

pllim commented 3 years ago

Alas, with the last few commits, the Cuts plot in Crosshair no longer works for me. The box is not appearing and nothing is plotted. I don't see any new warnings nor any errors with --loglevel=30 --stderr.

I tried with the following images: jbt7a3020_drz.fits and ngc6946.fits

If there is some way I can debug this on my end, please let me know.

ejeschke commented 3 years ago

I tried with the following images: jbt7a3020_drz.fits and ngc6946.fits

@pllim, downloaded both of these and tried them. No problem viewing them with Crosshair. Did you click the checkbox "Quick Cuts"?

ejeschke commented 3 years ago

If the box goes outside the data area the plot will not show. This is because it is not able to make an NxN cross cut. NaNs may also cause problems for the plot. But I didn't encounter any particular problems with the images you referred to above.

ejeschke commented 3 years ago

When I have the warn/alert limits set and I have it at a region that triggers one or both of them, if I zoom in too much, the yellow/red status disappears but comes back when I zoom back out again.

Ok, this should be fixed in the commit I just added. Actually clarified a question I had as to whether the default warning/alert mechanism should respond to data values that are in the plot but off of the visible portion of the graph. Answer: use the full data.

Sometimes the X and Y labels overlap each other (screenshot below).

I'm afraid this is more of a limitation of the plot layout and space constraints. I think it would be better addressed as a separate issue relating to plots, possibly. For now, the solution is to make the window wider.

ejeschke commented 3 years ago

Thanks, @pllim!