Update 1D Profile viewer to use wcsaxes for plotting and add sliders #2167

kakirastern · 2020-08-13T23:03:23Z

Description

To replace #2156.

Tasks accomplished with this PR:

Update the existing glue's 1D Profile viewer to use astropy's wcsaxes for plotting.
Add a 'slice' function option to the 1D Profile to enable plotting a 1D spectrum of a data cube (e.g. an IRIS raster data cube) at a position picked out by glue-solar's pixel extraction tool.
Add sliders to 1D Profile viewer to update plot.
Enable deletion of BaseData type data so that now the dataset in the data collection generated by glue-solar's pixel extraction tool can be deleted.

codecov · 2020-08-13T23:10:20Z

Codecov Report

Merging #2167 into master will decrease coverage by 0.14%.
The diff coverage is 79.05%.

@@            Coverage Diff             @@
##           master    #2167      +/-   ##
==========================================
- Coverage   87.89%   87.74%   -0.15%     
==========================================
  Files         246      248       +2     
  Lines       22764    22912     +148     
==========================================
+ Hits        20009    20105      +96     
- Misses       2755     2807      +52

Impacted Files	Coverage Δ
glue/viewers/image/qt/data_viewer.py	`100.00% <ø> (ø)`
glue/viewers/image/qt/profile_viewer_tool.py	`23.91% <ø> (+0.50%)`	⬆️
glue/viewers/image/qt/slice_widget.py	`71.87% <50.00%> (-0.76%)`	⬇️
glue/viewers/profile/qt/options_widget.py	`76.59% <54.54%> (-23.41%)`	⬇️
glue/viewers/profile/qt/slice_widget.py	`70.37% <70.37%> (ø)`
glue/viewers/profile/state.py	`95.28% <86.84%> (-3.55%)`	⬇️
glue/app/qt/layer_tree_widget.py	`80.22% <100.00%> (ø)`
glue/core/data.py	`90.64% <100.00%> (ø)`
glue/utils/wcs.py	`100.00% <100.00%> (ø)`
glue/viewers/common/qt/data_slice_widget.py	`83.21% <100.00%> (ø)`
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6634973...462c534. Read the comment docs.

astrofrog

This is looking good! In addition to the comments below:

The icon added looks identical to https://github.com/glue-viz/glue/blob/master/glue/icons/glue_crosshair.png?
There are also changes here that add the pixel extraction to the image viewer by default, which I'm not sure we want to do yet?

glue/app/qt/layer_tree_widget.py

glue/core/visual.py

glue/viewers/image/qt/data_viewer.py

glue/viewers/matplotlib/state.py

glue/viewers/profile/layer_artist.py

glue/viewers/profile/state.py

kakirastern · 2020-08-17T19:45:55Z

There are also changes here that add the pixel extraction to the image viewer by default, which I'm not sure we want to do yet?

Yeah, I could revert the changes made regarding the pixel extraction tool, but I would like to add the image to the glue repo so we could use it in glue-solar... Would that be okay?

kakirastern · 2020-08-17T22:41:56Z

Removed pixel extraction tool code, but kept the new image for glue-solar use.

kakirastern · 2020-08-20T09:42:06Z

@astrofrog PR ready for another review

glue/icons/icon_preview.html

Cadair · 2020-08-20T15:23:15Z

I have noticed two major usability issues with this:

When you move the sliders the view changes, it would be good if you zoom in to a region of interest and then move the sliders, the view stays where you put it.
When the function isn't slice, the sliders shouldn't be visible. This will really confuse people if they are there and useless.

kakirastern · 2020-08-20T18:03:12Z

So have just updated the 1D Profile viewer modules to show spectra as profile plots.

Regarding the comments:

I have noticed two major usability issues with this:

When you move the sliders the view changes, it would be good if you zoom in to a region of interest and then move the sliders, the view stays where you put it.

When the function isn't slice, the sliders shouldn't be visible. This will really confuse people if they are there and useless.

These will involve some deep changes and hence will need some time to accomplish.

kakirastern · 2020-08-20T18:14:39Z

But I reckon we could enhance this PR with regards to #2167 (comment) in a separate PR at a later stage.

kakirastern · 2020-08-20T23:42:24Z

Turned out the code I wrote was only working in this case only if one tweaks the opacity slider. (And it was working fine for sequentially stacked IRIS Level 2 datacubes.) And I have just fixed the underlying bugs so it is working fine for the typical FITS files containing data cubes as well. See the following for a demo:

kakirastern · 2020-08-22T16:26:18Z

Just added code to hide the extra sliders in the 1D Profile viewer's options widget when we have

self.viewer_state != 'slice'

astrofrog · 2020-08-24T08:31:21Z

This is looking good! I did run into an issue with the controls in the lower left when changing the x axis:

Do you see this too or shall I provide a data file to replicate this?

glue/viewers/image/qt/slice_widget.py

glue/viewers/profile/layer_artist.py

glue/viewers/profile/qt/options_widget.py

kakirastern · 2020-08-24T08:53:38Z

This is looking good! I did run into an issue with the controls in the lower left when changing the x axis:

Do you see this too or shall I provide a data file to replicate this?

Yeah, did you use the "spectrum tool" button? It is a known issue and I have a fix for it which I can implement it soon. I did meant to ask you but didn't get a chance to yet.

kakirastern · 2020-08-24T11:37:11Z

Yeah, did you use the "spectrum tool" button? It is a known issue and I have a fix for it which I can implement it soon. I did meant to ask you but didn't get a chance to yet.

Just patched this error. Actually I was baffled by why it was the case before as @Cadair never saw the extra sliders but I could. It was because he was using the drag-and-drop action to initiate the 1D Profile viewer. To tackle this error from using the spectrum tool button, I have just removed the code snippets that would cause such an issue in the commit da82536.

astrofrog

This is almost good to go! A few usability issues:

Currently the subset profiles don't seem to be right in slice mode, for instance if I make a small subset and push the sliders to the edge of the dataset then the subset profile should be empty/not shown:

Currently I can't get this to work with 2-d datasets - this used to work for non slice modes but appears broken for all modes now.
The font size in the profile viewer doesn't seem to be the same as for the image viewer:

I'll review the code in detail below.

glue/viewers/image/qt/profile_viewer_tool.py

glue/viewers/profile/layer_artist.py

glue/viewers/profile/qt/options_widget.py

astrofrog · 2020-09-01T12:16:23Z

glue/viewers/profile/qt/slice_widget.py

+            elif self._sliders[i] is None:
+                slices.append(0)
+
+        self.viewer_state.slices = slices


I think in future we'll want to try and avoid duplication with the image viewer - I can investigate once we add this to the 3-d viewer (to visualize >3-d datasets)

glue/viewers/profile/state.py

glue/viewers/profile/viewer.py

kakirastern · 2020-09-04T09:22:45Z

Have made the changes according to the suggestions given. PR is ready for another review.

astrofrog

Looks good to me! I'm now going to be a bit annoying and ask if you could add a test - a lot of the code here isn't covered by the test suite since there isn't a test for the profile viewer in slice mode. There are other tests in glue/viewers/profile/qt/tests you could perhaps start from ?

astrofrog · 2020-09-08T13:48:37Z

glue/viewers/profile/layer_artist.py

+        if force or any(prop in changed for prop in ('layer', 'slices', 'x_att', 'x_att_pixel', 'attribute',
+                                                     'function', 'normalize', 'v_min', 'v_max', 'visible')):
+            self._update_visual_attributes()
+            self._calculate_profile(reset=True)


For other viewers the approach is instead to do:

self._calculate_profile(reset=True) force=True

which ensures the visual properties are updated next. Maybe switch to this approach?

Tried changing to

self._calculate_profile(reset=True) self._update_profile(force=True)

But error was thrown upon attempting to view via 1D Profile viewer as data cannot be added for some reason...

glue/viewers/profile/state.py

glue/viewers/profile/tests/test_state.py

kakirastern · 2020-09-09T08:09:34Z

Looks good to me! I'm now going to be a bit annoying and ask if you could add a test - a lot of the code here isn't covered by the test suite since there isn't a test for the profile viewer in slice mode. There are other tests in glue/viewers/profile/qt/tests you could perhaps start from ?

Yup, sure. I could add some unit tests for the CI.

… to 1D Profile viewer

astrofrog requested changes Aug 16, 2020

View reviewed changes

kakirastern requested a review from astrofrog August 17, 2020 22:42

kakirastern mentioned this pull request Aug 19, 2020

Update pixel extraction tool to work with WCSAxes glue-viz/glue-solar#31

Closed

Cadair reviewed Aug 20, 2020

View reviewed changes

glue/icons/icon_preview.html Outdated Show resolved Hide resolved

astrofrog reviewed Aug 24, 2020

View reviewed changes

glue/viewers/image/qt/slice_widget.py Outdated Show resolved Hide resolved

glue/viewers/profile/layer_artist.py Outdated Show resolved Hide resolved

glue/viewers/profile/qt/options_widget.py Outdated Show resolved Hide resolved

kakirastern requested a review from astrofrog August 25, 2020 00:48

This was referenced Aug 25, 2020

[Experimental] Add SunPy 1D Profile plotting support glue-viz/glue-solar#21

Closed

Add tutorials to guide the user to use glue-solar for solar physics data glue-viz/glue-solar#19

Open

astrofrog requested changes Sep 1, 2020

View reviewed changes

kakirastern requested a review from astrofrog September 4, 2020 09:22

astrofrog requested changes Sep 8, 2020

View reviewed changes

Kris Stern added 5 commits September 16, 2020 08:39

Update 1D Profile viewer to use wcsaxes for plotting and add sliders

24ddbdc

Apply suggestions from code review

aabf3d7

Remove pixel extraction tool code

aff7477

Remove 'image:pixel_extraction' from list

7302e3c

Make changes to enable 1D profiles of 3D data cubes using profile viewer

085468b

Kris Stern added 9 commits September 16, 2020 08:39

Remove unused import statement

7272c10

Hide sliders unless function is 'slice' for profile viewer

3b82714

Define a new remove function to replace the hidden _clear function

62706dc

Tidy up

a7bbdf7

Remove unwanted extra sliders when transitioning from 2D Image viewer…

f981636

… to 1D Profile viewer

Apply changes as suggested in review

971ad8c

Patch test_state module for profile viewer after updating

c05af5f

Fix codestyle issues

67553e7

Add tests for 'slice' function'

462c534

kakirastern requested a review from astrofrog September 16, 2020 01:21

astrofrog force-pushed the master branch from 43a421e to d2fa267 Compare September 17, 2020 19:09

astrofrog deleted the branch glue-viz:master August 27, 2021 19:46

astrofrog closed this Aug 27, 2021

nabobalis mentioned this pull request Nov 9, 2021

Update 1D Profile viewer to use wcsaxes - Add slice function to 1D Profile viewer with sliders to select point #2248

Draft

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update 1D Profile viewer to use wcsaxes for plotting and add sliders #2167

Update 1D Profile viewer to use wcsaxes for plotting and add sliders #2167

kakirastern commented Aug 13, 2020

codecov bot commented Aug 13, 2020 •

edited

Loading

astrofrog left a comment

kakirastern commented Aug 17, 2020

kakirastern commented Aug 17, 2020

kakirastern commented Aug 20, 2020

Cadair commented Aug 20, 2020

kakirastern commented Aug 20, 2020

kakirastern commented Aug 20, 2020

kakirastern commented Aug 20, 2020

kakirastern commented Aug 22, 2020

astrofrog commented Aug 24, 2020

kakirastern commented Aug 24, 2020

kakirastern commented Aug 24, 2020

astrofrog left a comment

astrofrog Sep 1, 2020

kakirastern Sep 4, 2020

kakirastern commented Sep 4, 2020

astrofrog left a comment

astrofrog Sep 8, 2020

kakirastern Sep 9, 2020

kakirastern Sep 16, 2020

kakirastern commented Sep 9, 2020

Update 1D Profile viewer to use wcsaxes for plotting and add sliders #2167

Update 1D Profile viewer to use wcsaxes for plotting and add sliders #2167

Conversation

kakirastern commented Aug 13, 2020

Description

codecov bot commented Aug 13, 2020 • edited Loading

Codecov Report

astrofrog left a comment

Choose a reason for hiding this comment

kakirastern commented Aug 17, 2020

kakirastern commented Aug 17, 2020

kakirastern commented Aug 20, 2020

Cadair commented Aug 20, 2020

kakirastern commented Aug 20, 2020

kakirastern commented Aug 20, 2020

kakirastern commented Aug 20, 2020

kakirastern commented Aug 22, 2020

astrofrog commented Aug 24, 2020

kakirastern commented Aug 24, 2020

kakirastern commented Aug 24, 2020

astrofrog left a comment

Choose a reason for hiding this comment

astrofrog Sep 1, 2020

Choose a reason for hiding this comment

kakirastern Sep 4, 2020

Choose a reason for hiding this comment

kakirastern commented Sep 4, 2020

astrofrog left a comment

Choose a reason for hiding this comment

astrofrog Sep 8, 2020

Choose a reason for hiding this comment

kakirastern Sep 9, 2020

Choose a reason for hiding this comment

kakirastern Sep 16, 2020

Choose a reason for hiding this comment

kakirastern commented Sep 9, 2020

codecov bot commented Aug 13, 2020 •

edited

Loading