Add show() method as preferred entrypoint

[duytnguyendtn]

Description

Backstory

Ok I'm going to finally admit to a nearly 18 month secret I've been hiding since I originally wrote the first helper for specviz. I've always had negative opinions about viz.app. I feel like it's not intuitive and not apparent to a brand new user, and almost forces them to read the documentation first before even trying to take it for a spin (which I imagine some may see as a good thing, but I'm going to argue I don't have to read Chrome's documentation when I launch it!). So I added specviz.show() as a wrapper to the specviz.app call:

jdaviz/jdaviz/configs/specviz/helper.py

Lines 200 to 201 in 392e1b6

	def show(self):
	self.app

Here's the secret... it doesn't work. It never worked. I struggled for SO LONG at the time but I couldn't fix it; I just didn't understand what front-end wizardry made viz.app work.. So... I kept my mouth shut because I felt so strongly it should be there, and swore one day I would fix it. You can even see how old this is because it's only in the Specviz helper, and not in the base helper class; this was before I even wrote the base helper class, and the only helper we had was Specviz!

With @eteq's introduction of #952, I now further think this should be the way to show our viz tools, because a helper method would allow us to add options and arguments to a SINGLE entry point, rather than separate helpers for each launching mode (i.e. viz.show(mode=sidecar), viz.show(new_tab=True) rather than separate viz.show_in_new_tab() and viz.show_in_sidecar()

In my review of #952, I had a real bruhhh moment. The reason why it never worked was because I never returned the app...

... They say the mistakes of the past show how much you've grown. As an optimist, I choose to view it through this lens 😅 I'm legitimately giddy seeing this PR finally in fruition. Now I can finally put this issue to rest...

As a warning, I WILL die on top of this hill!! BYOP (Bring Your Own Pitchforks!)

tl;dr What actually changed??

This PR adds viz.show() as a helper method to the base ConfigHelper class, and updates all notebooks and documentation to show viz.show() rather than viz.app

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? Milestone is only currently required for PRs related to Imviz MVP.
• After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

[codecov]

Codecov Report

Merging #965 (82c0fe4) into main (a1d7d83) will decrease coverage by 0.07%.
The diff coverage is 52.00%.

@@            Coverage Diff             @@
##             main     #965      +/-   ##
==========================================
- Coverage   85.14%   85.07%   -0.08%     
==========================================
  Files          91       91              
  Lines        8325     8366      +41     
==========================================
+ Hits         7088     7117      +29     
- Misses       1237     1249      +12     

Impacted Files	Coverage Δ
jdaviz/configs/specviz/helper.py	56.62% <ø> (+0.15%)	⬆️
jdaviz/core/helpers.py	31.81% <52.00%> (-3.15%)	⬇️
jdaviz/configs/cubeviz/plugins/slice/slice.py	96.59% <0.00%> (-2.29%)	⬇️
...igs/default/plugins/model_fitting/model_fitting.py	78.13% <0.00%> (-0.35%)	⬇️
jdaviz/app.py	92.56% <0.00%> (-0.31%)	⬇️
...imviz/plugins/aper_phot_simple/aper_phot_simple.py	92.19% <0.00%> (-0.02%)	⬇️
jdaviz/core/astrowidgets_api.py	98.74% <0.00%> (ø)
jdaviz/core/template_mixin.py	91.42% <0.00%> (+0.07%)	⬆️
...configs/cubeviz/plugins/moment_maps/moment_maps.py	94.00% <0.00%> (+0.45%)	⬆️
...daviz/configs/default/plugins/collapse/collapse.py	100.00% <0.00%> (+1.96%)	⬆️
... and 1 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 a1d7d83...82c0fe4. Read the comment docs.

[PatrickOgle]

I have no objection, as long as viz.app still works, for back-compatibility in published notebooks.

[kecnry]

I have two hesitancies, which don’t necessarily mean you have to leave your hill:

1. Even though viz.app is still there, this hides that a bit which might make some advanced functionality a little more hidden (or maybe that’s a good thing since users will need to know what they’re doing before digging deeper)
   viz.show() doesn’t technically show anything, it just returns the app and the notebook interprets that as something to show.
2. If you were to run this outside a notebook, it would just return an object. Is there anyway to have this either raise a warning/error in python or (better yet) launch a popup window in a kernel?

[duytnguyendtn]

@PatrickOgle: Yes! This entirely maintains backwards compatibility. This is just an additional helper method, and as with all helper methods, it provides an additional, more human-accessible, entrpoint to jdaviz functionality
@kecnry Here's my thoughts to your points:

1. I feel that the helper's goal is to hide the base level viz.app! The whole point of the convenience helpers is to provide simple entrypoints to jdaviz functionality without having to drill down. Once you have to drill down, I'm going to say it's a feature that they need to read the docs for more advanced functionality
2. Actually, my aha! moment was assuming interpretation of the application class was exactly how viz.app worked! When I save a rendered notebook and try to open it up, both methods briefly show the returned Application object, before the app is actually rendered. I assumed this was exactly what you were saying, as the notebook interpreting the object as something to show. But if I'm wrong, please correct me! Oh frontend wizard, teach me your secrets!
   
3. Clarifying question about your last point: When outside the notebook context, the behavior of viz.show() and viz.app is equivalent here; both return the application itself. Are you suggesting that this shouldn't be the case for either and an exception should be thrown in both scenarios, or that this behavior is OK for viz.app and not viz.show()?

>>> viz.app
Application(components={'g-viewer-tab': '<template>\n  <component :is="stack.container">\n    <g-viewer-tab\n ...

>>> viz.show()
Application(components={'g-viewer-tab': '<template>\n  <component :is="stack.container">\n    <g-viewer-tab\n ...

[kecnry]

@duytnguyendtn - Exactly, the notebook knows to interpret that returned object and display it. I guess my slight issue is that using the "verb" show suggests that jdaviz is doing the showing. From the user-perspective of course, this distinction probably doesn't matter: in the notebook the end result is the UI is shown and no one cares how it happens. But, if you run the same lines in python (outside of a notebook), nothing shows which might be more confusing now because we hide this distinction behind the verb. Or if you do blah = viz.show() in a notebook, nothing is shown but rather is equivalent to blah = viz.app. Compare this to matplotlib's show, which will popup a standalone window from python and shows in a notebook regardless if you assign the output, for example. I don't think it's necessarily a dealbreaker, and think the pros likely outweigh the cons, I just wanted to bring that up for discussion/consideration.

[duytnguyendtn]

For an outside interpreter, I wonder if I can trigger the desktop app to fire with the user's instance of the viz tool. I feel that would adequately answer that usecase. It looks like we might want to add some "environment detection", i.e.:

if environment == notebook: rely on interpreter
elif environment == python (but outside notebook): start desktop app
...

I'm not sure what to do with blah = viz.show()... That one's tricky.

[camipacifici]

No objections. I do not see how our current users (even the advanced ones) would complain about this.
I would like to better understand what magic you can now do with show(), like can you actually feed a file to show() so that the app opens and the data are loaded with one command?

[kecnry]

@duytnguyendtn I like that idea, except that the desktop app would need to communicate with the original instance which was likely started without a kernel, so that might be quite difficult to implement in the current infrastructure. For now maybe its worth just detecting if the current session does not support showing and raise an error during viz.show() that points to viz.app to access the underlying object or suggests running in Jupyter? I don't think there's much we can do about the blah = viz.show() scenario unless we instead have show actually inject into the cell or something instead of just returning, but I also don't think its a showstopper and the advantages are probably well worth it.

[duytnguyendtn]

@kecnry I opted to raise a warning instead and return the app anyways just in case the user has an interactive environment that we don't know about. What do you think?

@camipacifici Ooh, I like the idea of viz.show(data="myfile.fits") as a oneliner to show both the data AND the viz tool! I don't think I'll implement that here, since that might be a larger discussion. I think the best manifestation of the improvements this PR brings (beyond the naming) can best be seen in #952. Erik has two new api methods to display the app in different ways (viz.show_in_new_tab() and viz.show_in_sidecar()). Notice how these are two separate methods. By having a central viz.show() we can avoid this, and rather just change the arguments (i.e. something like viz.show(mode=sidecar) or viz.show(mode=new_tab). But really, my main horse in this race is the naming clarity (i.e. viz.show() is more clear to the user, and also matches other python programs, like matplotlib.pyplot.show())

[ibusko]

Code looks good to me, and works.

[pllim]

Need a rebase to resolve conflicts.

[mariobuikhuizen]

@duytnguyendtn nice to see you got a secret off your chest 😄

You could also use display() to show the app and not return the app object (this is what implicitly happens with the last line of a notebook cell). This would also allow for .show() not having to be the last code line in a cell.

  def show(self):
    display(self.app)

[duytnguyendtn]

Moved to draft to address discussion for display modes referenced in #952 (comment)

[pllim]

Do we want to deprecate this?

[duytnguyendtn]

See #965 (comment)

[pllim]

Do we want to deprecate this?

[duytnguyendtn]

If we want to deprecate it, shouldn't we through a DeprecationWarning and leave it for a little bit?

[duytnguyendtn]

Ok, I think I addressed everything in #965 (review) so long as @pllim is okay with the Deprecation Warning? Since we removed the sidecar return, I also had to remove a lot of the coverage, and I think we're still trying to figure out how to test UI elements so I think the coverage will have to suffer a little bit to get this in. Otherwise, we're good for the final review!

[pllim]

I think we're very close. Just a few more comments. 😅

[pllim]

Does this test actually exposes a bug here? I thought you wanted "popout" to emit a NotImplementedError.

[pllim]

Let's get this in for the release. Thanks!

[duytnguyendtn]

Hehe... whoops, you beat me to the punch! Hopefully 82c0fe4 passes the tests!

So I was going to reply to your comment #965 (comment) that the reason I was trying to catch a RuntimeError is because I chained the exception to explicitly say that there was an error in the displaying; hence I wrap the original error with a generic RuntimeError with a more user friendly explanation.

[duytnguyendtn]

But that got me thinking the test should check for the chained NotImplementedError so I pushed I do have duytnguyendtn@aa658ea which changes that test to check for that chained NotImplementedError. Didn't quite make it to the merge. Did we want to merge that to resolve #965 (comment)?

[pllim]

Oh, ooops. Would you mind opening a follow-up PR then? If you want to change the kind of exception that is actually raised, should get it in before the release.

[duytnguyendtn]

On it!

[pllim]

p.s. Looking at your diff, I think it might be clearer to have those that raise NotImplementedError to be outside the try-except itself. You can check for those first before the rest of logic?