user API for slice plugin #1635

kecnry · 2022-09-09T16:33:53Z

Description

This pull request extends on #1401 and provides user-API access to the slice plugin.

TODO:

fix failing tests
add PR to changelog if before next release, otherwise add new entry (waiting until PR is accepted otherwise to avoid constant merge conflicts)

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 change log needed? If yes, is it added to CHANGES.rst?
Is a milestone set?
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

codecov · 2022-09-09T18:55:03Z

Codecov Report

Base: 86.34% // Head: 86.32% // Decreases project coverage by -0.01% ⚠️

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

❗ Current head c56b237 differs from pull request most recent head 4c22449. Consider uploading reports for the commit 4c22449 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1635      +/-   ##
==========================================
- Coverage   86.34%   86.32%   -0.02%     
==========================================
  Files          95       95              
  Lines        9622     9611      -11     
==========================================
- Hits         8308     8297      -11     
  Misses       1314     1314

Impacted Files	Coverage Δ
jdaviz/configs/cubeviz/plugins/slice/slice.py	`96.90% <100.00%> (+0.16%)`	⬆️
...default/plugins/metadata_viewer/metadata_viewer.py	`94.50% <0.00%> (-0.24%)`	⬇️
jdaviz/configs/imviz/plugins/catalogs/catalogs.py	`95.58% <0.00%> (-0.07%)`	⬇️
jdaviz/configs/default/plugins/viewers.py	`95.53% <0.00%> (-0.04%)`	⬇️
jdaviz/app.py	`91.79% <0.00%> (-0.02%)`	⬇️
...daviz/configs/default/plugins/collapse/collapse.py	`100.00% <0.00%> (ø)`

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.

javerbukh · 2022-09-14T16:21:21Z

jdaviz/configs/cubeviz/plugins/slice/slice.py

+      Wavelength of the current slice.  If setting, will update automatically to the nearest
+      slice.
+    * ``setting_show_indicator``
+      Whether to show indicator in spectral viewer even when slice tool is inactive.


Looks like this only changes the status of the indicator when it is inactive, is that the intended functionality?

yes. If it is set to True, the line changes from blue to orange based on whether the slice tool is active to click and drag. If it is set to False, the line disappears when inactive (but still appears and is orange when active). Any suggestions to clarify the wording (both here and in the UI)?

I think just removing the word "even" should be enough. Just wanted to clarify!

agreed that is simpler, made the change!

pllim · 2022-09-14T21:38:47Z

jdaviz/configs/cubeviz/plugins/slice/slice.py

+    * ``wavelength``
+      Wavelength of the current slice.  If setting, will update automatically to the nearest
+      slice.
+    * ``setting_show_indicator``


Now that this is public, should we remove the redundant "setting" prefix?

Suggested change

* ``setting_show_indicator``

* ``show_indicator``

I'm not sure. I have been using this prefix across all plugins for any switch that goes in the settings expansion panels of that plugin. But we can remove the prefix from all of them, or could invent a plugin.settings container to nest them?

Is that a lot of extra work? It is just a little confusing for me to see some with "settings" and some not. To me, they are all settings. I don't think API-wise, the user cares if it is nested in Settings panel or not.

I like having some distinction between switches that only affect visualization or how many details are shown, verse inputs that actually affect the output of the plugin. But maybe just ordering them in the API docs could accomplish that without the prefix or nesting. I'll give it some thought.

I support dropping the setting_ prefix, but don't feel too strongly about it.

ok, that would affect slice, spectral extraction, and plot options... none of which have been merged yet, so I'll update the other 2 open PRs separately as well.

jdaviz/configs/cubeviz/plugins/slice/slice.py

rosteen · 2022-09-15T15:14:49Z

jdaviz/configs/cubeviz/plugins/slice/slice.py

+      Wavelength of the current slice.  If setting, will update automatically to the nearest
+      slice.


Suggested change

Wavelength of the current slice. If setting, will update automatically to the nearest

slice.

Wavelength of the current slice. When setting this value directly, it will update

automatically to the value at the nearest integer slice index.

If we keep the setting_ prefix, this phrasing could cause some confusion (is "setting" here talking about the same thing as in the attribute names?). My suggestion might be a little overly wordy/precise, but might be clearer.

This was confusing, I committed a slightly modified version of your suggestion, let me know what you think.

New wording sounds good to me.

rosteen

Looks good to me now, thanks!

rosteen · 2022-09-15T15:36:23Z

Just needs a changelog update before merging. Edit: beat me to it, I hadn't refreshed!

kecnry added this to the 2.11 milestone Sep 9, 2022

github-actions bot added the cubeviz label Sep 9, 2022

kecnry force-pushed the api-slice branch 2 times, most recently from ef788a1 to 53c83be Compare September 9, 2022 18:43

kecnry marked this pull request as ready for review September 9, 2022 21:01

kecnry requested review from duytnguyendtn, rosteen, javerbukh, ojustino and pllim as code owners September 9, 2022 21:01

javerbukh reviewed Sep 14, 2022

View reviewed changes

kecnry force-pushed the api-slice branch from 53c83be to c56b237 Compare September 14, 2022 16:29

javerbukh approved these changes Sep 14, 2022

View reviewed changes

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

pllim reviewed Sep 14, 2022

View reviewed changes

rosteen reviewed Sep 15, 2022

View reviewed changes

rosteen approved these changes Sep 15, 2022

View reviewed changes

kecnry force-pushed the api-slice branch from 428ccca to f3afef3 Compare September 15, 2022 15:22

kecnry added 4 commits September 15, 2022 11:24

user API for slice plugin

1c27d24

remove setting_ prefix from traitlets

44b53d2

clarify description on wavelength traitlet

0950fd5

update changelog entry

4c22449

kecnry force-pushed the api-slice branch from f3afef3 to 4c22449 Compare September 15, 2022 15:25

kecnry merged commit 1cb9d95 into spacetelescope:main Sep 15, 2022

kecnry deleted the api-slice branch September 15, 2022 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

user API for slice plugin #1635

user API for slice plugin #1635

kecnry commented Sep 9, 2022 •

edited

Loading

codecov bot commented Sep 9, 2022 •

edited

Loading

javerbukh Sep 14, 2022

kecnry Sep 14, 2022

javerbukh Sep 14, 2022

kecnry Sep 14, 2022

pllim Sep 14, 2022

kecnry Sep 15, 2022

pllim Sep 15, 2022

kecnry Sep 15, 2022

rosteen Sep 15, 2022

kecnry Sep 15, 2022

rosteen Sep 15, 2022 •

edited

Loading

kecnry Sep 15, 2022

rosteen Sep 15, 2022

rosteen left a comment

rosteen commented Sep 15, 2022 •

edited

Loading

		Wavelength of the current slice. If setting, will update automatically to the nearest
		slice.

-      Wavelength of the current slice.  If setting, will update automatically to the nearest
-      slice.
+      Wavelength of the current slice.  When setting this value directly, it will update
+      automatically to the value at the nearest integer slice index.

user API for slice plugin #1635

user API for slice plugin #1635

Conversation

kecnry commented Sep 9, 2022 • edited Loading

Description

Checklist for package maintainer(s)

codecov bot commented Sep 9, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosteen Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosteen left a comment

Choose a reason for hiding this comment

rosteen commented Sep 15, 2022 • edited Loading

kecnry commented Sep 9, 2022 •

edited

Loading

codecov bot commented Sep 9, 2022 •

edited

Loading

rosteen Sep 15, 2022 •

edited

Loading

rosteen commented Sep 15, 2022 •

edited

Loading