Imviz: Allow Subset recentering via centroiding #1823

pllim · 2022-11-04T01:06:46Z

Description

Implements centroiding of Subset in Imviz.

This pull request also shows a proof-of-concept notebook on how we might be able to edit Subset objects programmatically in a fun way.

~~If you know of a better way to do this, please let me know.~~

Click here to watch it in action. (YouTube video)

Address part of #1442

Example workflow

At the beginning, an imperfectly centered Subset was drawn. Clicking on Recenter button will take you to the second screenshot.

Subset is moved. Editable fields (desired) agree with static fields (actual).

After about 10 iterations of this (recenter, recenter, ...), it finally converged; i.e., clicking "Recenter" will not change anything. Note that for some sources (e.g., crowded or noisy fields), it may never converge.

Change log entry

Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
should add a no-changelog-entry-needed label.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
Do the proposed changes follow the STScI Style Guides?
Are tests added/updated as required? If so, do they follow the STScI Style Guides?
Are docs added/updated as required? If so, do they follow the STScI Style Guides?
Did the CI pass? If not, are the failures related?
Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone.
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)? 🐱

codecov · 2022-11-04T01:17:31Z

Codecov Report

Base: 88.15% // Head: 88.23% // Increases project coverage by +0.08% 🎉

Coverage data is based on head (e7c15a4) compared to base (ab1b363).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1823      +/-   ##
==========================================
+ Coverage   88.15%   88.23%   +0.08%     
==========================================
  Files          95       95              
  Lines       10279    10343      +64     
==========================================
+ Hits         9061     9126      +65     
+ Misses       1218     1217       -1

Impacted Files	Coverage Δ
...igs/default/plugins/subset_plugin/subset_plugin.py	`98.54% <100.00%> (+0.61%)`	⬆️
...imviz/plugins/aper_phot_simple/aper_phot_simple.py	`91.83% <100.00%> (+<0.01%)`	⬆️
jdaviz/configs/imviz/plugins/viewers.py	`87.64% <100.00%> (+0.14%)`	⬆️
jdaviz/core/region_translators.py	`98.98% <100.00%> (+0.13%)`	⬆️
jdaviz/configs/mosviz/plugins/parsers.py	`91.13% <0.00%> (+0.02%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py

camipacifici · 2022-11-10T02:38:42Z

Love the functionality! I tried with multiple layers and everything worked as expected.
I find a bit inconvenient to have to click two things though. I do not see a use case where I would click recenter and not click update. Is there a better way to have the two actions happen with one click? If not, would you consider keeping the recenter tab open so at least the user does not have to click three times to do a second iteration?

pllim · 2022-11-10T03:19:29Z

I thought it would be too confusing to have "Recenter" do update and "Update" also do update. Only having one button that actually updates feel more consistent to me, but I'll defer to UI/UX experts.

would you consider keeping the recenter tab open

I would but I just don't know how to do that. Maybe Kyle knows? 😆

kecnry · 2022-11-10T14:05:44Z

I would but I just don't know how to do that. Maybe Kyle knows? 😆

Pushed a commit that'll do this (feel free to rename anything)

I find a bit inconvenient to have to click two things though. I do not see a use case where I would click recenter and not click update. Is there a better way to have the two actions happen with one click?

I think I agree with this, but also see how it might be useful to have feedback of what will happen before making the edit (currently irreversibly). Another option might be to have non-editable text in the plugin for the "proposed" changes to the subset information that updates in real time as the data selection or subset position is moved... but then we'd have that information in 3 different places in the plugin which might cause some confusion.

I think it would also be useful to have the data dropdown default to the top layer... but that might be a more general improvement to the data dropdown component itself and out-of-scope here?

kecnry · 2022-11-10T14:07:14Z

docs/imviz/plugins.rst

+in the app-level toolbar. It might not show some static regions loaded
 via the API unless an interactive region is drawn after.


is there an issue filed for fixing this?

I don't remember, maybe @rosteen does. It only happens if you load things like Space Invader (which turns into masked subset), so 99% of users will never even encounter this.

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py

kecnry · 2022-11-10T14:10:17Z

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py

+            reg = _get_region_from_spatial_subset(self, self.subset_selected)
+            aperture = regions2aperture(reg)


would it be useful to move this into the component itself for re-use - so self.subset_select.aperture or self.subset_select.to_aperture()? Or would it have a place upstream in glue-astronomy?

I think this will eventually move upstream. At least Larry or I added a few of these in that table.

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py

camipacifici · 2022-11-10T14:56:26Z

Having the tab open is very useful thank you!
I was thinking that another workflow could be to do one recenter and one update to see that the subset is actually moving in the right direction (no mistake in the layer or subset or anything), but then I could keep pressing recenter until the numbers do not change anymore before updating again. As far as I understand, this is currently not possible. I have to update before I can recenter again. Can "keep clicking recenter" be an option?

pllim · 2022-11-10T16:48:50Z

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.vue

@@ -22,6 +22,29 @@
      </v-col>
    </v-row>

+    <!-- Sub-plugin for recentering of spatial subset (Imviz only) -->
+    <v-row v-if="config=='imviz' && is_editable">
+      <v-expansion-panels accordion v-model="subplugins_opened">


Magical. Thanks, Kyle!

pllim · 2022-11-10T16:52:41Z

I think it would also be useful to have the data dropdown default to the top layer

I wanted that a long time ago but never got it to work. See #743

pllim · 2022-11-10T16:59:13Z

Can "keep clicking recenter" be an option?

I don't see how that is useful without visual feedback. You see the numbers keep changing but the subset is not moving. How do you know it is moving in the right direction?

pllim · 2022-11-10T17:20:32Z

@kecnry and @camipacifici , have I sufficiently addressed/answered your comments/questions?

camipacifici · 2022-11-11T17:52:49Z

Can "keep clicking recenter" be an option?

I don't see how that is useful without visual feedback. You see the numbers keep changing but the subset is not moving. How do you know it is moving in the right direction?

I would Guess that if the user sees it moving once in the right direction they will know it is going in the right direction when seeing the numbers change. And they could still update at any time.

camipacifici · 2022-11-11T17:54:52Z

If I understood correctly, now I can keep pressing recenter, but nothing happens and there is no instructions that I have to update before I can recenter again (or maybe I missed it?). User might think that centroiding has converged, while it has not.

kecnry · 2022-11-11T17:59:38Z

One option around this might be to have the recenter button immediately update the subset but to cache the old version and have a "revert/undo" button that can undo the latest step in case the user doesn't like the changes?

camipacifici · 2022-11-11T18:39:59Z

I like the solution, if you think it will not be confusing for the user to have an "undo" button only here (and in the zoom tool right?), but not everywhere.

pllim · 2022-11-11T19:01:41Z

Then what do we do with the Update button? I am getting a little worried.

kecnry · 2022-11-11T19:09:32Z

Then what do we do with the Update button?

No harm would be done if the user clicks "update" again, right? But maybe @Jenneh would have some ideas to make the different use-cases intuitive. Or maybe we're overthinking and recentering will almost never do anything "wrong" and so we can just immediately update the subset without having to worry about the user wanting to reject the proposed changes?

for Imviz only. Fix GUI update and dithered use case.

Increase test coverage.

pllim · 2022-11-11T19:44:13Z

It's Friday and it appears I am outvoted 2-to-1, so I went ahead and changed the Recenter button to also Update.

kecnry

Recentering itself seems to work as expected! But I guess subplugins_opened needs to be Any in order to accept None when closed 🤷 (sorry I missed that earlier).

There is some UI flashing when updating the subset, but that appears even with the manual entry so can probably be considered out of scope. Also out of scope - this is another case where making the layer/data dropdowns intelligent about the top layer (either by having a "top layer" option in the dropdown, having them default, or show a warning when selecting something that is not the top layer) would be quite useful!

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py

Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>

pllim · 2022-11-14T17:12:03Z

Thanks! I applied your suggested fix.

camipacifici · 2022-11-14T17:41:56Z

this is another case where making the layer/data dropdowns intelligent about the top layer (either by having a "top layer" option in the dropdown, having them default, or show a warning when selecting something that is not the top layer) would be quite useful!

I agree!

camipacifici · 2022-11-14T17:42:19Z

Works well! Thank you!

pllim · 2022-11-14T17:47:17Z

having a "top layer" option in the dropdown

But top layer in which viewer? 😉 Pondering for another day.

I'll count Cami's comment as second approval and merge. Thanks, all!

rosteen · 2022-11-14T17:51:49Z

I was too slow in testing this, but for what it's worth I also approve 😆. I like the immediate update on hitting the recenter button.

pllim added the no-changelog-entry-needed changelog bot directive label Nov 4, 2022

pllim added this to the 3.2 milestone Nov 4, 2022

pllim requested review from duytnguyendtn, rosteen, javerbukh, ojustino, kecnry and bmorris3 as code owners November 4, 2022 01:06

github-actions bot added the documentation Explanation of code and concepts label Nov 4, 2022

pllim added the imviz label Nov 4, 2022

pllim marked this pull request as draft November 7, 2022 20:37

pllim force-pushed the pong branch from 2817498 to dd1b93a Compare November 7, 2022 22:19

pllim removed the no-changelog-entry-needed changelog bot directive label Nov 7, 2022

pllim changed the title ~~POC: Programmatically edit subsets~~ POC: Programmatically recenter subsets Nov 7, 2022

pllim force-pushed the pong branch from dd1b93a to be14727 Compare November 8, 2022 23:49

pllim commented Nov 8, 2022

View reviewed changes

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py Outdated Show resolved Hide resolved

pllim changed the title ~~POC: Programmatically recenter subsets~~ Imviz: Allow Subset recentering via centroiding Nov 9, 2022

pllim force-pushed the pong branch from 93fa0ed to 484d64d Compare November 9, 2022 19:56

pllim marked this pull request as ready for review November 9, 2022 23:17

kecnry reviewed Nov 10, 2022

View reviewed changes

pllim commented Nov 10, 2022

View reviewed changes

pllim and others added 7 commits November 11, 2022 14:13

POC: Programmatically edit subsets

d5bdeaf

Nicer API for centering

4ea27bc

WIP: Sub-plugin to recenter subset

ac4e828

for Imviz only. Fix GUI update and dithered use case.

Added tests

404b5c5

keep expansion panel opened when redrawn

f052469

Address Kyle comments

fd7f671

Recenter now also move the Subset.

c425f7e

Increase test coverage.

pllim force-pushed the pong branch from 3e30b93 to c425f7e Compare November 11, 2022 19:43

pllim requested a review from Jenneh November 14, 2022 15:51

kecnry reviewed Nov 14, 2022

View reviewed changes

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py Outdated Show resolved Hide resolved

jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py Outdated Show resolved Hide resolved

Fix subplugin when close

e7c15a4

Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>

kecnry approved these changes Nov 14, 2022

View reviewed changes

kecnry added the Ready for final review label Nov 14, 2022

pllim merged commit f0796f2 into spacetelescope:main Nov 14, 2022

pllim deleted the pong branch November 14, 2022 17:47

pllim mentioned this pull request Nov 17, 2022

DOC: Clarify centroid subtlety #1854

Merged

9 tasks

kecnry mentioned this pull request Dec 20, 2022

Manual backport PR #1908: set bounding box to None to avoid layer cropping #1942

Merged

stscijgbot-jwql mentioned this pull request Jul 9, 2024

Option to disable centroiding in Simple Aperture Photometry #1442

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Imviz: Allow Subset recentering via centroiding #1823

Imviz: Allow Subset recentering via centroiding #1823

pllim commented Nov 4, 2022 •

edited

Loading

codecov bot commented Nov 4, 2022 •

edited

Loading

camipacifici commented Nov 10, 2022

pllim commented Nov 10, 2022

kecnry commented Nov 10, 2022

kecnry Nov 10, 2022

pllim Nov 10, 2022

kecnry Nov 10, 2022

pllim Nov 10, 2022

camipacifici commented Nov 10, 2022

pllim Nov 10, 2022

pllim commented Nov 10, 2022

pllim commented Nov 10, 2022

pllim commented Nov 10, 2022

camipacifici commented Nov 11, 2022

camipacifici commented Nov 11, 2022

kecnry commented Nov 11, 2022

camipacifici commented Nov 11, 2022

pllim commented Nov 11, 2022

kecnry commented Nov 11, 2022

pllim commented Nov 11, 2022

kecnry left a comment

pllim commented Nov 14, 2022

camipacifici commented Nov 14, 2022

camipacifici commented Nov 14, 2022

pllim commented Nov 14, 2022

rosteen commented Nov 14, 2022

		in the app-level toolbar. It might not show some static regions loaded
		via the API unless an interactive region is drawn after.

		reg = _get_region_from_spatial_subset(self, self.subset_selected)
		aperture = regions2aperture(reg)

Imviz: Allow Subset recentering via centroiding #1823

Imviz: Allow Subset recentering via centroiding #1823

Conversation

pllim commented Nov 4, 2022 • edited Loading

Description

Example workflow

Change log entry

Checklist for package maintainer(s)

codecov bot commented Nov 4, 2022 • edited Loading

Codecov Report

camipacifici commented Nov 10, 2022

pllim commented Nov 10, 2022

kecnry commented Nov 10, 2022

kecnry Nov 10, 2022

Choose a reason for hiding this comment

pllim Nov 10, 2022

Choose a reason for hiding this comment

kecnry Nov 10, 2022

Choose a reason for hiding this comment

pllim Nov 10, 2022

Choose a reason for hiding this comment

camipacifici commented Nov 10, 2022

pllim Nov 10, 2022

Choose a reason for hiding this comment

pllim commented Nov 10, 2022

pllim commented Nov 10, 2022

pllim commented Nov 10, 2022

camipacifici commented Nov 11, 2022

camipacifici commented Nov 11, 2022

kecnry commented Nov 11, 2022

camipacifici commented Nov 11, 2022

pllim commented Nov 11, 2022

kecnry commented Nov 11, 2022

pllim commented Nov 11, 2022

kecnry left a comment

Choose a reason for hiding this comment

pllim commented Nov 14, 2022

camipacifici commented Nov 14, 2022

camipacifici commented Nov 14, 2022

pllim commented Nov 14, 2022

rosteen commented Nov 14, 2022

pllim commented Nov 4, 2022 •

edited

Loading

codecov bot commented Nov 4, 2022 •

edited

Loading