Added initial developer docs on linking

[astrofrog]

Description

I've discussed various aspects of linking with some of you in Slack but I thought it would be good to write up some developer docs about the most recent state of linking and what I think should be best practices going forward with linking. I plan to expand this over time - and whenever someone asks me a question about linking that is not covered by this page, I will do by best to add some information to the page.

In principle we could improve this page by adding intersphinx links to the glue docs for classes and methods, but I've run out of time for today - I'll gladly accept suggestions/commits that add intersphinx links, or I can try and add it later this week or next week at some point (but would rather we merge this and then fix the intersphinx links later than delay merging for a while).

Please do ask questions about anything that is unclear during review or things that you think are missing!

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 #811 (6c7a4ff) into main (4d38952) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #811   +/-   ##
=======================================
  Coverage   67.21%   67.21%           
=======================================
  Files          65       65           
  Lines        4612     4612           
=======================================
  Hits         3100     3100           
  Misses       1512     1512           

---

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 4d38952...6c7a4ff. Read the comment docs.

[pllim]

How do you "throw away the links" and rebuild, as stated in #735 ?

[astrofrog]

How do you "throw away the links" and rebuild, as stated in #735 ?

I'm glad you asked! Let me add a section about that 😄

[astrofrog]

@pllim - done! Does this make sense?

[pllim]

"Powerful, convincing, and most ingenious." ⭐⭐⭐⭐⭐

Proof-reading comments and nitpicks below.

[pllim]

As per comment above about intersphinx.

Suggested change

	:class:`~glue.plugins.wcs_autolinker.wcs_autolinker.WCSLink` while two other
	``glue.plugins.wcs_autolinker.wcs_autolinker.WCSLink``, while two other

[pllim]

Suggested change

	should not be linked both by :class:`~glue.plugins.wcs_autolinker.wcs_autolinker.WCSLink`
	should not be linked by both WCS

[pllim]

Needs a rebase. Conflict in index.rst.

I'll re-review once I hear back about your API linking investigation. Thanks!

[pllim]

One more question. What is the proper way to do such a thing with a table of markers when Imviz links by pixels only? Do I have to "throw away" existing links or is what I already have okay?

jdaviz/jdaviz/configs/imviz/helper.py

Lines 517 to 524 in 01731c0

	if use_skycoord:
	if not data_has_valid_wcs(image):
	raise AttributeError(f'{getattr(image, "label", None)} does not have a valid WCS')
	sky = table[skycoord_colname]
	t_glue = Data(marker_name, ra=sky.ra.deg, dec=sky.dec.deg)
	jglue.data_collection[marker_name] = t_glue
	jglue.add_link(t_glue, 'ra', image, 'Right Ascension')
	jglue.add_link(t_glue, 'dec', image, 'Declination')

[astrofrog]

@pllim - I've fixed the intersphinx links so will resolve all your comments that are purely intersphinx-related.

[astrofrog]

What is the proper way to do such a thing with a table of markers when Imviz links by pixels only?

If there is just one image dataset, what you have is fine (though note that the code you have will not work for non-equatorial images). If you have multiple image datasets that are offset, then how do you choose which one the markers should line up with? (since the stars in different images won't line up). I don't think there is an unambiguous answer to that.

[astrofrog]

@pllim - ready for another review.

[pllim]

Re: linking markers -- Good question and maybe @eteq can comment too. A similar conversation came up in #741 today between us. Currently, I link the markers table with a "reference image".

jdaviz/jdaviz/configs/imviz/helper.py

Lines 512 to 514 in 4d38952

	# TODO: How to link to invidual images separately for X/Y? add_link in loop does not work.
	# Link markers to reference image data.
	image = viewer.state.reference_data

Conceptually over at astrowidgets (which is not aware of linking or blinking), markers are tied to the image being displayed on a particular viewer. So, we're just winging here when markers in reality are global across Imviz. Better ideas to handle markers are certainly welcome.

[pllim]

Thanks! I will merge when CI passes. I think one approval is enough.