aperture photometry: do not hide prev results when resetting #2112

kecnry · 2023-03-23T19:38:11Z

Description

This pull request prevents resetting the internal traitlets in aperture photometry when failing to find a dataset/subset or raising an error, and only clears them when needed for the next successful run. This also then keeps the log table visible.

Fixes #2111

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 · 2023-03-23T19:47:49Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.05 🎉

Comparison is base (5510486) 91.90% compared to head (de06d52) 91.95%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2112      +/-   ##
==========================================
+ Coverage   91.90%   91.95%   +0.05%     
==========================================
  Files         143      143              
  Lines       15605    15665      +60     
==========================================
+ Hits        14342    14405      +63     
+ Misses       1263     1260       -3

Impacted Files	Coverage Δ
...imviz/plugins/aper_phot_simple/aper_phot_simple.py	`92.87% <100.00%> (+0.65%)`	⬆️

... and 4 files with indirect coverage changes

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

kecnry · 2023-03-23T19:49:04Z

@pllim - I originally was just going to move the table out of the v-if, but then realized that we probably want everything to act the same and just keep showing until there are new results. Can you think of any workflow/case where this isn't the intended/desired behavior?

And I'm not sure how to milestone or where to put the changelog - does this count as a bugfix or a change in behavior?

pllim · 2023-03-24T14:49:44Z

Will not clearing get confusing at some point? You have result from previous run, but then unsuccessful but don't bother reading the snackbar. Now you still see numbers and you are in a rush, won't you mistaken the numbers for the new run?

pllim · 2023-03-24T14:51:28Z

jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.py

@@ -249,7 +238,6 @@ def _bg_subset_selected_changed(self, event={}):

    def vue_do_aper_phot(self, *args, **kwargs):
        if self._selected_data is None or self._selected_subset is None:
-            self.reset_results()


I still think you need to reset when new calculation is requested.

can you show a case where that is needed? I think everything just gets overwritten for a successful run and the code is redundant, but I'm not 100% sure.

The point is, when this button is pressed, things should be cleared and only show new calculated results. If there is any failure at all (this, or the except Exception as e below), no numbers should show, to make it clear that something has gone wrong.

The logic here should not affect the issue you are trying to fix. Deleting a Subset should not trigger this method.

Or am I misunderstanding the situtation?

Deleting all subsets does trigger resetting the results and ends up hiding both the most recent results and the entire table. I can move the entire table outside of that logic and to its own logic (just checking the length of the table or something), but that feels like extraneous logic and inconsistent behavior to me.

Screen.Recording.2023-03-24.at.11.13.49.AM.mov

Delete subset event triggers vue_do_aper_phot? 🤯

No, but the subset selection itself becomes blank, and then _subset_selected_changed clears the results.

Okay, we can stop clearing for that. But I am saying it should still clear within vue_do_aper_phot.

Should all the previous results (including the table) disappear? What if instead we show a message that the previous run failed above all of those so they are not lost?

Ok, so what if we show the error message in the UI in place of the results and keep the table?

This might be a good enough compromise.

kecnry · 2023-03-24T14:57:22Z

You have result from previous run, but then unsuccessful but don't bother reading the snackbar. Now you still see numbers and you are in a rush, won't you mistaken the numbers for the new run?

Ok, so what if we show the error message in the UI in place of the results and keep the table?

javerbukh

Tried this a few times adding and deleting subsets and all data stayed in the table and the results from the previous run persisted until there was a new subset selected for analysis. All of that makes sense to me!

pllim

Thanks!

…v results when resetting

…2-on-v3.4.x Backport PR #2112 on branch v3.4.x (aperture photometry: do not hide prev results when resetting)

do not hide prev results when resetting

0e7f50a

github-actions bot added the imviz label Mar 23, 2023

pllim reviewed Mar 24, 2023

View reviewed changes

kecnry added 2 commits March 27, 2023 09:16

show in-plugin warning text if last "calculate" call failed

4b44ecf

changes entry

de06d52

kecnry added this to the 3.4.1 milestone Mar 27, 2023

kecnry marked this pull request as ready for review March 27, 2023 13:29

kecnry requested review from duytnguyendtn, rosteen, javerbukh, bmorris3 and haticekaratay as code owners March 27, 2023 13:29

javerbukh approved these changes Mar 27, 2023

View reviewed changes

javerbukh added the Ready for final review label Mar 27, 2023

pllim approved these changes Mar 27, 2023

View reviewed changes

pllim added the 💤backport-v3.4.x on-merge: backport to v3.4.x label Mar 27, 2023

pllim merged commit 4cd8c30 into spacetelescope:main Mar 27, 2023

meeseeksmachine mentioned this pull request Mar 27, 2023

Backport PR #2112 on branch v3.4.x (aperture photometry: do not hide prev results when resetting) #2115

Merged

meeseeksmachine pushed a commit to meeseeksmachine/jdaviz that referenced this pull request Mar 27, 2023

Backport PR spacetelescope#2112: aperture photometry: do not hide pre…

fd219cc

…v results when resetting

kecnry deleted the aper-phot-keep-prev-results branch March 27, 2023 17:50

pllim added a commit that referenced this pull request Mar 27, 2023

Merge pull request #2115 from meeseeksmachine/auto-backport-of-pr-211…

bd28042

…2-on-v3.4.x Backport PR #2112 on branch v3.4.x (aperture photometry: do not hide prev results when resetting)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

aperture photometry: do not hide prev results when resetting #2112

aperture photometry: do not hide prev results when resetting #2112

kecnry commented Mar 23, 2023

codecov bot commented Mar 23, 2023 •

edited

Loading

kecnry commented Mar 23, 2023

pllim commented Mar 24, 2023

pllim Mar 24, 2023

kecnry Mar 24, 2023

pllim Mar 24, 2023

kecnry Mar 24, 2023

pllim Mar 24, 2023

kecnry Mar 24, 2023

pllim Mar 24, 2023

kecnry Mar 24, 2023

pllim Mar 24, 2023

kecnry commented Mar 24, 2023

javerbukh left a comment

pllim left a comment

aperture photometry: do not hide prev results when resetting #2112

aperture photometry: do not hide prev results when resetting #2112

Conversation

kecnry commented Mar 23, 2023

Description

Change log entry

Checklist for package maintainer(s)

codecov bot commented Mar 23, 2023 • edited Loading

Codecov Report

kecnry commented Mar 23, 2023

pllim commented Mar 24, 2023

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

kecnry commented Mar 24, 2023

javerbukh left a comment

Choose a reason for hiding this comment

pllim left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 23, 2023 •

edited

Loading