user API for links control

[kecnry]

Description

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

Plugin User API docs

TODO:

• 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]

Codecov Report

Base: 87.11% // Head: 87.12% // Increases project coverage by +0.00% 🎉

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

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1640   +/-   ##
=======================================
  Coverage   87.11%   87.12%           
=======================================
  Files          95       95           
  Lines        9887     9894    +7     
=======================================
+ Hits         8613     8620    +7     
  Misses       1274     1274           

Impacted Files	Coverage Δ
...nfigs/imviz/plugins/links_control/links_control.py	100.00% <100.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.

[pllim]

I am also not sure about this one. This plugin exposes an existing API. This feels... recursive.

https://jdaviz.readthedocs.io/en/latest/api/jdaviz.configs.imviz.helper.link_image_data.html

[javerbukh]

I'll leave the last word up to @pllim but this looks good to me!

[pllim]

It still feels weird to me to let user have a proper API imviz.link_data() but also let them do it via plugin API...

[kecnry]

@pllim - so what do you propose? I think our options would be:

• having this be one of the only plugins without an exposed API
• make the helper-method private
• if the API lives in the helper, then maybe the functionality for the UI should not be in the plugin tray and should be moved to the app-level toolbar
• live with it in both places, but try to isolate plugin code to plugins going forward

[pllim]

having this be one of the only plugins without an exposed API

It is a possibility and easy to do. People who use API wouldn't use this plugin anyway. This plugin is only for those who does not want to use API or is stuck in the "standalone" app.

make the helper-method private

No. It is already advertised as public.

maybe the functionality for the UI should not be in the plugin tray and should be moved to the app-level toolbar

It is also possible but more work. We could have a pixel/WCS toggle but with approx always on. To also add in approx would introduce 3 options, not sure how toggle works with that. 🤷

try to isolate plugin code to plugins going forward

Isn't this the first option?

[kecnry]

People who use API wouldn't use this plugin anyway. This plugin is only for those who does not want to use API or is stuck in the "standalone" app.

I don't entirely agree with this. I suspect many people will use the API first and then try to write reproducible code that will walk through those same steps. The plugin APIs provide an easy transition to them, as the APIs mimic the exact same options available in the plugin. So if someone uses the plugin to change linking, then the plugin API for that is probably easier to find/learn than the helper method with different syntax. If someone starts using Jdaviz with the API, then they may find the helper method easier to access.

try to isolate plugin code to plugins going forward

Isn't this the first option?

What I mean is that if we were creating this plugin now, I would advocate for not having the helper-method at all, or at least not public. But I agree, in this case it already does exist, so we should avoid removing it. I think that alone is an excuse for having the two ways to do the same thing... and we just need to keep this in mind as we develop new functionality to try to avoid it in the future. But ultimately some things will move from plugins to core functionality, or vice versa, and we can't always predict that.

[pllim]

Personally, I would find it annoying if the existing helper API is removed. What I can now do in one line, I would need multiple lines to accomplish using the plugin API.

[kecnry]

What I can now do in one line, I would need multiple lines to accomplish using the plugin API.

I think this is a good thing to keep in mind going forward as well. If we were designing this from scratch now (which we're not), we could have a similar helper-method at the plugin level to pass multiple options and update the linking. There are similar calls to that implemented in the spectral extraction plugin now, and might be a good pattern to try to follow elsewhere.

But we're not designing this from scratch and already have the method in the helper and the UI in the plugin... my opinion is that duplicated functionality is less confusing than having the exposed API lacking for the plugin just because it exists elsewhere as well 🤷 but I'm curious to hear what others think as well.

[rosteen]

I think my take is that it is ok to both consistently expose APIs for all of the plugins, and have convenient helper functions for commonly used things that are perhaps shorter or more concise to use than the plugin API itself.

[pllim]

Okay, let's get this in then but please add a change log first. Thanks!

[kecnry]

ah, lost my chance to squash & merge 😢 😂

[pllim]

Ooops, sorry. It is only 3 commits though and they look atomic enough. 😬