large subset warnings in aperture photometry

[kecnry]

Description

This pull request is to address slow performance for the raw profile plot in aperture photometry when using large subsets. This does two things the following to attempt to address this case:

1. reorders so the results table appears above the plots. Any size image/subset that jdaviz currently handles should return the results almost immediately... if this is ever not the case, then we should consider adding a spinner while computing the results.
2. adds a warning below the dropdown box if the user selected the raw profile AND the selected aperture has an area of over 5000 pixels. This is a somewhat arbitrary cutoff, but can be adjusted if reviewers/users see significant performance degradation at smaller subsets.

Screen.Recording.2022-11-01.at.1.34.08.PM.mov

Unfortunately a spinner for just the plot isn't obviously feasible as the drawing is handled in bqplot and the browser... although it might be possible to implement from javascript callbacks if we want to explore that further.

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)?

[pllim]

I have a feeling that this number actually depends on the hardware. Is "5000" representative enough? Should we document how you decided on this number?

[kecnry]

It likely is, I kind of just guessed, but as I said in the description, I'm hoping that we can revise that during reviews. Ultimately it isn't a cutoff where we prevent running, just a cutoff where we start displaying the warning, so I think we should err on the side of too low of a number... or maybe we should just always display the message?

[codecov]

Codecov Report

Base: 87.89% // Head: 87.92% // Increases project coverage by +0.02% 🎉

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

❗ Current head 3604062 differs from pull request most recent head 1d5060e. Consider uploading reports for the commit 1d5060e to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1801      +/-   ##
==========================================
+ Coverage   87.89%   87.92%   +0.02%     
==========================================
  Files          95       95              
  Lines       10186    10211      +25     
==========================================
+ Hits         8953     8978      +25     
  Misses       1233     1233              

Impacted Files	Coverage Δ
...imviz/plugins/aper_phot_simple/aper_phot_simple.py	91.82% <100.00%> (+0.04%)	⬆️
jdaviz/configs/specviz/plugins/viewers.py	85.46% <0.00%> (+1.29%)	⬆️

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]

I tried using a very large subset (75k+) and it crashed the tab I have jupyter-lab running in. Tested on main and the same thing happens. I think including a warning is good but do we want to consider setting a hard limit on subset size as a followup?

[kecnry]

do we want to consider setting a hard limit on subset size as a followup?

We definitely can, but the actual limit probably depends on the memory available to the browser and will vary from machine to machine (I suppose maybe we could detect and estimate that limit that on the javascript-side, or maybe we could request that bqplot handles refusing to overload the memory).

[pllim]

Instead of warning, do we just want to exclude listing "large" subsets for Aperture Photometry plugin only?

[camipacifici]

How would you define "large"?

[pllim]

@camipacifici , currently it is arbitrarily subset_area > 5000 as stated in the diff.

[camipacifici]

I did miss that comment, thanks. Anyway, I meant the motivation to pick a specific number. I would not exclude subsets. The warning seems fine for now. We can revisit this later if we get reports of the app crashing.

[pllim]

@larrybradley specifically wanted the fit results to be on top, so moving this down is a step backwards. See #1409 (comment)

[kecnry]

ok, thanks for that context. This does keep them with the plot, but moves the plot and the gaussian fit below the photometry results. If we want to move them both back up to the top, then we might need to look into a javascript solution for detecting when the plot is rendered and show a spinner in the meantime, since the blank plot just suggests something isn't working.

[pllim]

Moving things around need PO's blessing, I think. I am neutral on whatever that works.

The warning itself pops up as advertised though it is hard for me to judge whether 5000 is a good cutoff or not.

[camipacifici]

Maybe this is a Jenn question, but would you consider having two buttons? calculate photometry and plot curve or something like that, so the user can decide not to show either if they do not need it and they would see first what they want.

[kecnry]

I like that idea, and then the warning is more tied to just the plotting step rather than computing the photometry itself - would you like me to do that as part of this PR?

[pllim]

I originally wanted the plotting to be a separate plugin...

I think adding another button would introduce too much diff that goes beyond the scope of this PR. Is there a way to do the warning without moving things around?

[kecnry]

Yes, I can easily revert changing the order but keep the warning, but see my concerns above for why I think this isn't ideal. But if the plan is to follow-up (relatively soon) with splitting into two buttons/sections, then the warning on its own is still an incremental improvement.

I'm also curious what @larrybradley thinks - was your original request from the comment @pllim linked just to keep the gaussian results tied closely with the plots, or to have the plot and gaussian results before the photometry results?

[camipacifici]

Stick with the warning for this PR and open a new one (if you have time now or an issue if you don't) for splitting the button, please.

[kecnry]

I did revert the re-ordering for this PR. I'm working on some smarter thresholds for the warning and will then open a follow-up ticket for either considering re-ordering, splitting the buttons, or splitting the plugin.

[pllim]

As discussed, this is a good minimal implementation of the request. We can fancy it up later.

Works on dark mode! 🎉

[pllim]

I'll let you decide if a change log is needed.

[kecnry]

Woops, change log got lost when I removed the re-ordering commit. Re-added and will merge once CI passes. Thanks for the reviews!