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.

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

Rotate image in Imviz #1340

Closed
wants to merge 4 commits into from
Closed

Conversation

pllim
Copy link
Contributor

@pllim pllim commented May 23, 2022

Description

This pull request is to allow image rotation in Imviz.

Still need a lot of some work but here is what I get with the stuff in the concept notebook added in this PR:

Screenshot 2022-06-07 172839

Bonus: Compass zoom box now can rotate too.

Screenshot 2022-06-08 155529

TODO

  • How to make Compass play nice with rotation? We will just disable the zoom box in Compass when this plugin is active or something.
  • How to hide the reference WCS but keep it as reference?
  • How will this work when multiple images are loaded?
  • How will this work with GWCS?
  • How to undo/redo rotation?
  • Performance considerations.
  • Manually test a variety of other workflows: Multiple viewers, etc.
  • When POC works, implement in plugin.
  • Clean up concept notebook.
  • Bug: Coordinates info bar can get out of sync after rotating with 3 images in the concept notebook. Maybe my Lab is messed up? Try in notebook and see.
  • Take out the stuff moved to BUG: Imviz bug fixes ported from PR 1340 #1392
  • Write unit tests.

ref #1466

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)? 🐱

@pllim pllim added this to the 2.6 milestone May 23, 2022
@github-actions github-actions bot added documentation Explanation of code and concepts imviz labels May 23, 2022
@codecov
Copy link

codecov bot commented May 23, 2022

Codecov Report

Merging #1340 (1466bf0) into main (8c3d3e7) will decrease coverage by 0.40%.
The diff coverage is 60.81%.

❗ Current head 1466bf0 differs from pull request most recent head 6aea214. Consider uploading reports for the commit 6aea214 to get more accurate results

@@            Coverage Diff             @@
##             main    #1340      +/-   ##
==========================================
- Coverage   84.91%   84.50%   -0.41%     
==========================================
  Files          91       93       +2     
  Lines        8288     8398     +110     
==========================================
+ Hits         7038     7097      +59     
- Misses       1250     1301      +51     
Impacted Files Coverage Δ
...configs/imviz/plugins/rotate_image/rotate_image.py 43.54% <43.54%> (ø)
jdaviz/core/template_mixin.py 91.21% <50.00%> (-0.12%) ⬇️
jdaviz/configs/imviz/wcs_utils.py 66.07% <53.84%> (-0.60%) ⬇️
jdaviz/configs/imviz/plugins/viewers.py 77.48% <69.56%> (-2.99%) ⬇️
jdaviz/app.py 92.46% <75.00%> (-0.29%) ⬇️
jdaviz/configs/imviz/helper.py 98.71% <100.00%> (+0.01%) ⬆️
jdaviz/configs/imviz/plugins/__init__.py 100.00% <100.00%> (ø)
...s/imviz/plugins/line_profile_xy/line_profile_xy.py 99.04% <100.00%> (+0.07%) ⬆️
...viz/configs/imviz/plugins/rotate_image/__init__.py 100.00% <100.00%> (ø)

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 8c3d3e7...6aea214. Read the comment docs.

@pllim pllim force-pushed the twist-and-shout branch from a02a380 to 35b208d Compare May 24, 2022 16:01
@rosteen rosteen modified the milestones: 2.6, 2.7 May 25, 2022
@astrofrog
Copy link
Collaborator

The approach of using a dummy dataset with a rotated WCS is 'what I would do too given the current glue infrastructure. I think what we ideally want to be able to do is to have the ability to have such datasets have a special status in glue which would make them 'hidden' from most lists except for the reference data in the image viewer, but that's not possible for now. But it is something I could prioritize if desired.

@pllim

This comment was marked as resolved.

@pllim pllim force-pushed the twist-and-shout branch 2 times, most recently from 3f98fa0 to 556ccb2 Compare June 3, 2022 15:27
@pllim pllim force-pushed the twist-and-shout branch 3 times, most recently from b7b066b to feb9850 Compare June 7, 2022 22:33
@pllim pllim force-pushed the twist-and-shout branch from feb9850 to c6838be Compare June 8, 2022 16:30
# Imviz falls back to ID for user-created viewers.
if viewer_item is not None:
viewer = self._viewer_store[viewer_item['id']]
else: # Maybe it is ID
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used to have this in the app somewhere but maybe it was removed in one of the refactoring. @kecnry , I know your pet peeve is the reference vs ID stuff, so I want to make sure you are okay with this change.

@pllim pllim force-pushed the twist-and-shout branch from c6838be to d7e55d4 Compare June 8, 2022 17:49
@pllim pllim force-pushed the twist-and-shout branch from d7e55d4 to 1466bf0 Compare June 8, 2022 21:00
@pllim pllim mentioned this pull request Jun 9, 2022
20 tasks
@pllim
Copy link
Contributor Author

pllim commented Jun 10, 2022

Looks like this PR might be rejected, so I am going to take out the bug fixes and open new PR for that (see #1392).

pllim added a commit to pllim/jdaviz that referenced this pull request Jun 10, 2022
pllim added a commit to pllim/jdaviz that referenced this pull request Jun 10, 2022
@pllim
Copy link
Contributor Author

pllim commented Jun 14, 2022

From tag up this morning, POs @orifox and @larrybradley have agreed to pursue #1384 instead. Therefore, I moved the bug fixes to #1392 and closing this PR without merge.

@pllim pllim closed this Jun 14, 2022
@pllim pllim deleted the twist-and-shout branch June 14, 2022 16:37
@pllim pllim mentioned this pull request Jul 7, 2022
pllim added a commit to pllim/jdaviz that referenced this pull request Jan 21, 2023
that still does not work
@kecnry kecnry mentioned this pull request Jan 26, 2023
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Explanation of code and concepts imviz
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Imviz coordinates display shows wrong X and Y for dithered image on load
3 participants