Skip to content

Implement set_viewport/get_viewport in interface, tests, and dummy viewer #46

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

mwcraig
Copy link
Member

@mwcraig mwcraig commented May 30, 2025

@mwcraig mwcraig marked this pull request as draft May 30, 2025 22:56
@mwcraig mwcraig requested a review from Copilot May 30, 2025 22:56
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements new viewport functionality by adding set_viewport and get_viewport methods to the interface and their corresponding implementations in the dummy viewer, as well as comprehensive tests to verify the correct behavior and error handling.

  • Expanded tests in widget_api_test.py to cover both pixel and world coordinate viewport settings and error conditions.
  • Updated interface_definition.py to define the new viewport methods with detailed parameter checks and documentation.
  • Refactored dummy_viewer.py to implement viewport handling during image loading and adjustments.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/astro_image_display_api/widget_api_test.py Adds tests for viewport setting/getting and robust error checking for invalid inputs.
src/astro_image_display_api/interface_definition.py Updates the API interface to include set_viewport/get_viewport with comprehensive docs.
src/astro_image_display_api/dummy_viewer.py Implements viewport management and modifies image-loading flows to integrate the viewport.
Comments suppressed due to low confidence (2)

src/astro_image_display_api/dummy_viewer.py:451

  • The variable 'point' is undefined in this context; consider using an existing variable (such as the computed 'center') or removing this line.
self._center = point

src/astro_image_display_api/dummy_viewer.py:466

  • The offset_by method is expected to update the viewer's state rather than return a dictionary; adjust the implementation to perform an in-place update.
return dict(
            center=center,
            fov=fov,
            wcs=viewport.wcs,
            image_label=image_label
        )

"""
Center the view on the point.
Set the viewport of the image, which defines the center and field of viea.
Copy link
Preview

Copilot AI May 30, 2025

Choose a reason for hiding this comment

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

There is a spelling mistake in the docstring; 'viea' should be corrected to 'view'.

Suggested change
Set the viewport of the image, which defines the center and field of viea.
Set the viewport of the image, which defines the center and field of view.

Copilot uses AI. Check for mistakes.

@mwcraig mwcraig force-pushed the set-get-viewport branch from 05a57f6 to fc521b1 Compare May 31, 2025 20:16
@mwcraig mwcraig requested a review from Copilot May 31, 2025 20:17
@mwcraig mwcraig marked this pull request as ready for review May 31, 2025 20:17
@mwcraig mwcraig requested review from eteq and pllim May 31, 2025 20:18
@mwcraig mwcraig marked this pull request as draft May 31, 2025 20:18
@mwcraig
Copy link
Member Author

mwcraig commented May 31, 2025

Need to add image_label to set/get stretch and cuts...

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a unified viewport API (set_viewport/get_viewport) by updating the interface, implementing it in the dummy viewer, and adding comprehensive tests.

  • Introduced set_viewport/get_viewport in interface_definition.py, replacing legacy view methods.
  • Extended widget_api_test.py with tests covering pixel/world centers, FOVs, and error cases.
  • Enhanced dummy_viewer.py to track per-image viewport state and support image_label.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
widget_api_test.py Added detailed tests for the new viewport methods
interface_definition.py Defined set_viewport/get_viewport in the viewer protocol
dummy_viewer.py Implemented viewport storage, image_label resolution, and logic
Comments suppressed due to low confidence (1)

src/astro_image_display_api/dummy_viewer.py:194

  • The _load_fits method reads the CCDData but doesn’t initialize the viewport (center & fov). It should call set_viewport with appropriate defaults so get_viewport returns valid values after loading.
def _load_fits(self, file: str | os.PathLike, image_label: str | None) -> None:

@mwcraig mwcraig force-pushed the set-get-viewport branch from fc521b1 to 90a6526 Compare June 1, 2025 15:18
@mwcraig mwcraig requested a review from Copilot June 1, 2025 15:18
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds set_viewport/get_viewport functionality to the ImageViewerInterface, implements it in ImageViewer (dummy viewer), and updates tests to cover pixel/world viewport handling.

  • Introduced image_label parameter across load_image, set_viewport, and get_viewport.
  • Replaced the old center_on/offset_by/zoom API with unified set_viewport and get_viewport.
  • Expanded tests in widget_api_test.py for setting/getting center and FOV in both pixel and world coordinates.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
src/astro_image_display_api/widget_api_test.py Expanded and corrected tests for set_viewport/get_viewport; updated imports and data shape.
src/astro_image_display_api/interface_definition.py Updated the interface to declare set_viewport/get_viewport; removed old methods.
src/astro_image_display_api/dummy_viewer.py Implemented set_viewport/get_viewport logic and image‐label bookkeeping in the dummy viewer.
Comments suppressed due to low confidence (4)

src/astro_image_display_api/widget_api_test.py:226

  • The call to self.image.load_image(data) in test_set_get_view_port_no_image_label is over-indented, causing a syntax/indentation error. Align it with the other statements in the test block.
      self.image.load_image(data)

src/astro_image_display_api/widget_api_test.py:221

  • [nitpick] The test name test_set_get_view_port_no_image_label uses view_port instead of viewport, which is inconsistent with other test names. Rename it to test_set_get_viewport_no_image_label.
    def test_set_get_view_port_no_image_label(self, data):

src/astro_image_display_api/interface_definition.py:139

  • This comment refers to "catalog labels" but should reference "image labels" to match the API terminology.
                # No user-defined catalog labels, so return the default label.

src/astro_image_display_api/interface_definition.py:149

  • Grammatical error in the message: change "a image_label" to "an image_label", and consider replacing "catalog styles" with "images" for clarity.
                        "Multiple catalog styles defined. Please specify a image_label to get the style."

@mwcraig mwcraig force-pushed the set-get-viewport branch from 90a6526 to 75090b2 Compare June 1, 2025 15:50
@mwcraig mwcraig requested a review from Copilot June 1, 2025 15:50
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements new viewport functionality (set_viewport/get_viewport) for the image viewer interface and updates the tests and dummy viewer implementation accordingly. Key changes include:

  • Updated test cases in widget_api_test.py to exercise new viewport, FOV, and error-handling logic.
  • Modified the interface definition to include optional image labels for image loading, cuts, and stretch settings.
  • Revised the dummy viewer to manage viewport state via a new ViewportInfo class and implement the updated API methods.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/astro_image_display_api/widget_api_test.py Updates tests to verify new viewport behavior, including both pixel and world coordinate modes and proper error handling when image labels are missing or invalid.
src/astro_image_display_api/interface_definition.py Adjusts method signatures and docstrings to include image labels and new viewport parameters, deprecating the old center_on/offset_by API.
src/astro_image_display_api/dummy_viewer.py Refactors image loading and viewport management with the introduction of ViewportInfo and updates to set_viewport/get_viewport methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

What does zoom level mean when viewer has no concept of pixels? Proposal: Add way to get center of viewer
1 participant