Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Footprints plugin: handle subsets/markers when linking by WCS #3276

Merged
merged 5 commits into from
Nov 11, 2024
Merged

Footprints plugin: handle subsets/markers when linking by WCS #3276

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4e3617c
update alert language in orientation plugin
kecnry Nov 8, 2024
9a85a6b
footprints: if changing link type fails, redirect to orientation plugin
kecnry Nov 8, 2024
be76347
changelog
kecnry Nov 8, 2024
789c5c5
if-else instead of try-except
kecnry Nov 11, 2024
4a4677d
Apply suggestions from code review
kecnry Nov 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGES.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,11 @@ Imviz

- Remove "From File.." option when running on an external server. [#3239]

- Button in the footprints plugin to change the link-type now redirects to the orientation plugin
when the change fails due to the presence of subsets or markers. [#3276]

- Updates UI language in the orientation plugin to better match API. [#3276]

Mosviz
^^^^^^

Expand Down
6 changes: 5 additions & 1 deletion jdaviz/configs/imviz/plugins/footprints/footprints.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,11 @@ def vue_link_by_wcs(self, *args):
# call other plugin so that other options (wcs_fast_approximation, wcs_use_fallback)
# are retained. Remove this method if support for plotting footprints
# when pixel-linked is reintroduced.
self.app._jdaviz_helper.plugins['Orientation'].align_by = 'WCS'
op = self.app._jdaviz_helper.plugins['Orientation']
if op._obj.need_clear_astrowidget_markers or op._obj.need_clear_subsets:
op.open_in_tray()
else:
op.align_by = 'WCS'

def _ensure_first_overlay(self):
if not len(self._overlays):
Expand Down
17 changes: 11 additions & 6 deletions jdaviz/configs/imviz/plugins/orientation/orientation.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,20 @@
</v-alert>

<v-alert
v-if="wcs_linking_available"
v-if="wcs_linking_available && !need_clear_astrowidget_markers && !need_clear_subsets"
type='warning'
class="ignore-api-hints"
style="margin-left: -12px; margin-right: -12px"
>
Switching link type will reset zoom.
Switching alignment will reset zoom.
</v-alert>

<v-alert
v-if="plugin_markers_exist && !need_clear_astrowidget_markers && !need_clear_subsets"
type='warning'
style="margin-left: -12px; margin-right: -12px"
>
Marker positions may not be pixel-perfect when changing alignment/linking options.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was there before, I think from concerns by @pllim

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(the diff here is just from moving its position so that the "warnings" both come before the alerts with actions)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot the exact reason now but hopefully that concern was captured somewhere in this GitHub repo. Maybe related to this?

jdaviz/jdaviz/configs/imviz/tests/test_orientation.py

Lines 74 to 81 in 5130caf

# Both marks stay the same in sky, so this means X and Y w.r.t. reference
# same on both entries.
# FIXME: 0.25 offset introduced by fake WCS layer (remove AssertionError).
# https://jira.stsci.edu/browse/JDAT-4256
with pytest.raises(AssertionError):
assert_allclose(mp._obj.marks["imviz-0"].x, 0)
with pytest.raises(AssertionError):
assert_allclose(mp._obj.marks["imviz-0"].y, 0)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, I think you're right. That is still open, so let's leave the text in place and I'll make sure there is a link back here so that we remove the warning if/when addressing that. Thanks!

</v-alert>

<v-alert v-if="need_clear_astrowidget_markers" type='warning' style="margin-left: -12px; margin-right: -12px">
Expand All @@ -38,12 +46,9 @@
</v-row>
</v-alert>

<v-alert v-if="plugin_markers_exist" type='warning' style="margin-left: -12px; margin-right: -12px">
Marker positions may not be pixel-perfect when changing alignment/linking options.
</v-alert>

<v-alert v-if="need_clear_subsets" type='warning' style="margin-left: -12px; margin-right: -12px">
Existing subsets will be deleted on changing alignment/linking options.
Existing subsets must be deleted before changing alignment/linking options.
<v-row justify="end" style="margin-right: 2px; margin-top: 16px">
<v-btn @click="delete_subsets">
{{ api_hints_enabled ?
Expand Down
Loading