Fix reference data for WCS to Pixels to WCS

[pllim]

Description

I corrected the test first. Will push commit(s) out later when I can figure out how to fix it properly.

Fixes #2724

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. Bugfix milestone also needs an accompanying backport label.
• After merge, any internal documentations need updating (e.g., JIRA, Innerspace)? 🐱 👻

[pllim]

Is this going in circles?

jdaviz/jdaviz/configs/imviz/plugins/orientation/orientation.py

Lines 410 to 413 in deda388

	@observe('orientation_layer_selected')
	def _change_reference_data(self, *args, **kwargs):
	if self._refdata_change_available:
	self.app._change_reference_data(

triggers

jdaviz/jdaviz/app.py

Line 546 in deda388

self.hub.broadcast(ChangeRefDataMessage(

that is being listened back in

jdaviz/jdaviz/configs/imviz/plugins/orientation/orientation.py

Lines 122 to 123 in deda388

	self.hub.subscribe(self, ChangeRefDataMessage,
	handler=self._on_refdata_change)

that calls this

jdaviz/jdaviz/configs/imviz/plugins/orientation/orientation.py

Line 437 in deda388

self.orientation.selected = msg.data.label

that would trigger this again?

jdaviz/jdaviz/configs/imviz/plugins/orientation/orientation.py

Lines 410 to 413 in deda388

	@observe('orientation_layer_selected')
	def _change_reference_data(self, *args, **kwargs):
	if self._refdata_change_available:
	self.app._change_reference_data(

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.72%. Comparing base (9edd445) to head (f1ba4a6).
Report is 17 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2751      +/-   ##
==========================================
+ Coverage   88.70%   88.72%   +0.01%     
==========================================
  Files         108      110       +2     
  Lines       16305    16353      +48     
==========================================
+ Hits        14464    14509      +45     
- Misses       1841     1844       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[rosteen]

Is this going in circles?

I think it could potentially get called one extra time, but the second time self.orientation.selected wouldn't actually change so it wouldn't trigger another loop.

[pllim]

Theoretically Orientation is the only plugin that is allowed to change the ref data, right? There is no reason for it to listen to its own message?

[rosteen]

You can also change the ref data through the viewer data menu, so the Orientation plugin needs to see those events to stay in synch.

[pllim]

Right... 😒 Thanks for the reminder!

[rosteen]

Drats, I thought there was a good chance that this also fixed the bug I was trying to fix on Friday, but it didn't. I did confirm that it fixes the intended use case though. I'll take a look at the actual code before approving.

[pllim]

the actual code

Sorry for the messy diff.

[rosteen]

Looks good to me, I tested a couple more edge cases. I left one comment about an error message but other than that the code looks fine.

[pllim]

Code suggestion accepted. Thanks!

[rosteen]

LGTM now.

[cshanahan1]

I tested this out in ImvizDitheredExample and it works well for me.

There was one instance when I was messing around with it that when you did some combination of linking, adding and removing viewers, etc, that after that it because very laggy (particularly with subset creation, they were taking 5+ seconds to appear after drawing), but I can't seem to recreate that same scenario.

Would it be possible to add additional test coverage? A lot of the lines are not covered

[pllim]

OK looks like the lines are some safeguards that didn't get triggered by test suite. I can look into that as part of this PR but I should probably rebase first. 🤞

UPDATE: Never mind. The uncovered lines are basically impossible to reach via plugin safeguards but does not hurt to keep them. I just marked them as uncovered for now.

[pllim]

xref #2755

[pllim]

Change log failure is unrelated but something I need to fix anyway: scientific-python/action-check-changelogfile#13

[pllim]

Clare verbally approved the PR at tag up this morning, so I am just going to merge when CI is green.

[cshanahan1]

haha yes i retroactively approve :)