Bqplot with new api - #191
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
Introduce a new bqplot-based backend for the ImageViewerInterface, add tests, and disable the legacy ginga import.
- Add full bqplot implementation in
astrowidgets/bqplot.py - Add basic instantiation and interface tests for the new
ImageWidget - Update
setup.cfgto suppress unavoidable warnings and comment out the ginga import in__init__.py
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| setup.cfg | Ignore specific DeprecationWarning/UserWarning from traitlets |
| astrowidgets/tests/test_widget_api_bqplot.py | Add instantiation and interface tests for ImageWidget |
| astrowidgets/bqplot.py | Implement bqplot backend, plotting, zoom, pan, and save APIs |
| astrowidgets/init.py | Comment out import of the ginga backend |
Comments suppressed due to low confidence (1)
astrowidgets/tests/test_widget_api_bqplot.py:1
- [nitpick] Tests currently cover only instantiation and interface conformance. Consider adding tests for image loading, colormap setting, and marker plotting to ensure core behaviors of the new backend are covered.
import pytest
Comment on lines
+300
to
+302
| # The width is used here but the height could be used instead | ||
| # and the result would be the same since the pixels are square. | ||
| scale_width = self.scale_widths[1] |
There was a problem hiding this comment.
get_current_width uses scale_widths[1], which is the vertical height, not the horizontal width. Use scale_widths[0] to return the horizontal dimension.
Suggested change
| # The width is used here but the height could be used instead | |
| # and the result would be the same since the pixels are square. | |
| scale_width = self.scale_widths[1] | |
| # The width is used here and matches the horizontal dimension. | |
| # Using the correct index ensures clarity and consistency with the docstring. | |
| scale_width = self.scale_widths[0] |
Comment on lines
+508
to
+511
| # THIS IS TERRIBLE AND MAKES THINGS SUPER LAGGY!!!! Needs to be | ||
| # throttled or something. Look at the ImageGL observe options. | ||
| x_scale.observe(update_zoom_level, names='max') | ||
|
|
There was a problem hiding this comment.
[nitpick] This comment is very emphatic and suggests a temporary workaround. Consider implementing proper throttling or a more controlled observer to avoid performance issues and remove the all-caps emphasis.
Suggested change
| # THIS IS TERRIBLE AND MAKES THINGS SUPER LAGGY!!!! Needs to be | |
| # throttled or something. Look at the ImageGL observe options. | |
| x_scale.observe(update_zoom_level, names='max') | |
| # Observing changes to the x scale without throttling can cause | |
| # performance issues. Implementing throttling or debouncing is necessary. | |
| x_scale.observe(self._throttled_update_zoom_level(), names='max') | |
| def _throttled_update_zoom_level(self, func, delay=0.1): | |
| """ | |
| Throttle the update_zoom_level function to prevent frequent calls. | |
| """ | |
| last_call_time = [0] | |
| def wrapper(event): | |
| current_time = time.time() | |
| if current_time - last_call_time[0] > delay: | |
| last_call_time[0] = current_time | |
| func(event) | |
| return wrapper |
Merged
mwcraig
added a commit
to mwcraig/astrowidgets
that referenced
this pull request
Sep 13, 2025
mwcraig
added a commit
to mwcraig/astrowidgets
that referenced
this pull request
Sep 13, 2025
mwcraig
added a commit
that referenced
this pull request
Sep 15, 2025
A few minor changes in response to the copilot review of #191
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I'm hoping to get this merged in an hour or so and release it even though it is not really totally ready and it comments out the ginga import.