8000 Move export ROI implementation into qt_viewer by Czaki · Pull Request #7950 · napari/napari · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Move export ROI implementation into qt_viewer #7950

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

Jump to bottom
Open
wants to merge 32 commits into
base: main
Choose a base branch
from
Open

Move export ROI implementation into qt_viewer #7950

wants to merge 32 commits into from
+295 −200

Conversation

Czaki
Copy link
Collaborator
@Czaki Czaki commented May 19, 2025 •
< 8000 div class="position-relative"> edited
Loading

References and relevant issues

Description

In #7803 I got a few segfaults of test_export_rois. This PR refactor export_roi feature into QtViewer and change test to use qt_viewer fixture instead of make_napari_viewer. This should reduce fragility of tests and allo to use export_roi feature with qt_viewer embedded in custom window (as a nice additional feature)

Note

The error to easier count dots when reproducing

   src\napari\_qt\_tests\test_qt_viewer.py ..........................s [3952/4910]
  ..Windows fatal exception: access violation

@Czaki Czaki added this to the 0.6.2 milestone May 19, 2025
@Czaki Czaki added the maintenance PR with maintance changes, label May 19, 2025
@github-actions github-actions bot added tests Something related to our tests qt Relates to qt labels May 19, 2025
@codecov Codecov
Copy link
codecov bot commented May 19, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 97.54098% with 3 lines in your changes missing coverage. Please review.

Project coverage is 93.03%. Comparing base (3365795) to head (d571004).
Report is 12 commits behind head on main.

Files with missing lines Patch % Lines
src/napari/_qt/qt_viewer.py 97.43% 2 Missing ⚠️
src/napari/_qt/qt_main_window.py 75.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #7950      +/-   ##
==========================================
+ Coverage   92.93%   93.03%   +0.10%     
==========================================
  Files         647      648       +1     
  Lines       60975    61466     +491     
==========================================
+ Hits        56665    57187     +522     
+ Misses       4310     4279      -31

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Czaki Czaki modified the milestone: 0.6.2 May 21, 2025
@Czaki Czaki requested a review from a team as a code owner May 21, 2025 07:43
@willingc
Copy link
Collaborator

@Czaki Does this mitigate or work around the Qt bug?

@Czaki
Copy link
Collaborator Author
Czaki commented May 21, 2025

In general, our teardown procedure for make_napari_viewer is fragile. But when I tried to refactor it in #7780, I got a slower solution, that failed in other points.

This PR workaround it by using Qt Viewer and pure Qt stack for deletion

willingc
willingc approved these changes May 22, 2025
View reviewed changes
Copy link
Collaborator
@willingc willingc left a comment

Choose a reason for hiding this comment

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

This looks like it simplifies things @Czaki. In an airport so didn't run locally.

brisvag
brisvag approved these changes May 23, 2025
View reviewed changes
8000
Copy link
Contributor
@brisvag brisvag left a comment

Choose a reason for hiding this comment

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

LGTM! It feel slike this should actually just mostly live on the ViewerModel to be honest, but we can do that at a later point!

@TimMonko TimMonko added the ready to merge Last chance for comments! Will be merged in ~24h label May 24, 2025
@Czaki
Copy link
Collaborator Author
Czaki commented May 25, 2025

Its again crash in test_export_rois on windows

@Czaki Czaki removed the ready to merge Last chance for comments! Will be merged in ~24h label May 25, 2025
@jni
Copy link
Member
jni commented May 27, 2025

LOL I looked at the code and approved before seeing the last comment 😂 🤦 😢

@TimMonko
Copy link
Contributor

I'm going to do a lame drive-by and wanted to check this env locally. I don't get a fatal error but do get an error suggesting we aren't adjusting for scaling correctly.

c:\Users\Tim\napari\src\napari\_qt\_tests\test_qt_viewer.py::test_export_rois failed: qt_viewer = <napari._qt.qt_viewer.QtViewer object at 0x000001E8401CEB70>
viewer_model = ViewerModel(camera=Camera(center=(0.0, np.float64(49.5), np.float64(49.5)), zoom=np.float64(4.332), angles=(0.0, 0.0, ...on dims_scroll at 0x000001E83DF50180>], _persisted_mouse_event={}, _mouse_drag_gen={}, _mouse_wheel_gen={}, _keymap={})
tmp_path = WindowsPath('C:/Users/Tim/AppData/Local/Temp/pytest-of-Tim/pytest-6/test_export_rois0')
qtbot = <napari.conftest.QtBotWithOnCloseRenaming object at 0x000001E83EC97380>

    def test_export_rois(qt_viewer, viewer_model, tmp_path, qtbot):
        # Create an image with a defined shape (100x100) and a square in the middle
    
        img = np.zeros((100, 100), dtype=np.uint8)
        img[25:75, 25:75] = 255
    
        # Add viewer
        viewer_model.add_image(img, colormap='gray')
    
        # Create a couple of clearly defined rectangular polygons for validation
        roi_shapes_data = [
            np.array([[0, 0], [20, 0], [20, 20], [0, 20]]) - (0.5, 0.5),
            np.array([[15, 15], [35, 15], [35, 35], [15, 35]]) - (0.5, 0.5),
            np.array([[65, 65], [85, 65], [85, 85], [65, 85]]) - (0.5, 0.5),
            np.array([[15, 65], [35, 65], [35, 85], [15, 85]]) - (0.5, 0.5),
            np.array([[65, 15], [85, 15], [85, 35], [65, 35]]) - (0.5, 0.5),
            np.array([[40, 40], [60, 40], [60, 60], [40, 60]]) - (0.5, 0.5),
        ]
        paths = [
            str(tmp_path / f'roi_{i}.png') for i in range(len(roi_shapes_data))
        ]
    
        # Save original camera state for comparison later
        camera_center = viewer_model.camera.center
        camera_zoom = viewer_model.camera.zoom
    
        with pytest.raises(ValueError, match='The number of file'):
            qt_viewer.export_rois(roi_shapes_data, paths=paths + ['fake'])
        # Export ROI to image path
        test_roi = qt_viewer.export_rois(roi_shapes_data, paths=paths)
    
        assert all(
            (tmp_path / f'roi_{i}.png').exists()
            for i in range(len(roi_shapes_data))
        )
    
        # This test uses scaling to adjust the expected size of ROI images
        # and number of white pixels in the ROI screenshots
        # The assertion may fail if the test is run on screens with fractional scaling.
        scaling = qt_viewer.screen().devicePixelRatio()
    
        assert all(
            roi.shape == (20 * scaling, 20 * scaling, 4) for roi in test_roi
        )
        assert viewer_model.camera.center == camera_center
        assert viewer_model.camera.zoom == camera_zoom
    
        test_dir = tmp_path / 'test_dir'
        qt_viewer.export_rois(roi_shapes_data, paths=test_dir)
        QApplication.processEvents()
        qtbot.wait(10)
        assert all(
            (test_dir / f'roi_{i}.png').exists()
            for i in range(len(roi_shapes_data))
        )
        expected_values = [0, 100, 100, 100, 100, 400]
        for index, roi_img in enumerate(test_roi):
            gray_img = roi_img[..., 0]
>           assert (
                np.count_nonzero(gray_img) == expected_values[index] * scaling**2
            ), f'Wrong number of white pixels in the ROI {index}'
E           AssertionError: Wrong number of white pixels in the ROI 1
E           assert 168 == (100 * (1.25 ** 2))
E            +  where 168 = <function count_nonzero at 0x000001E81903B9B0>(array([[  0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,   0,\n          0,   0,   0,   0,   0,   0,   0,   ...   0,   0,   0,   0,   0, 255,\n        255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255, 255]],\n      dtype=uint8))
E            +    where <function count_nonzero at 0x000001E81903B9B0> = np.count_nonzero

src\napari\_qt\_tests\test_qt_viewer.py:392: AssertionError

@Czaki
Copy link
Collaborator Author
Czaki commented May 29, 2025

I'm going to do a lame drive-by and wanted to check this env locally. I don't get a fatal error but do get an error suggesting we aren't adjusting for scaling correctly.

Yes. When I write this code I think about integer scaling, not fractional.
This code will work with 150% scaling, but not working with 125% as 10px*125% = 12.5px so it is rendered as 13px length.

Will fix it later but do not want to touch this code before this PR is finished.

@Czaki Czaki mentioned this pull request May 29, 2025
Open
@Czaki Czaki mentioned this pull request May 30, 2025
Merged
@TimMonko TimMonko modified the milestones: 0.6.2, 0.6.3 Jun 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jni jni jni approved these changes

@willingc willingc willingc approved these changes

@brisvag brisvag brisvag approved these changes

@DragaDoncila DragaDoncila Awaiting requested review from DragaDoncila

@psobolewskiPhD psobolewskiPhD Awaiting requested review from psobolewskiPhD

Assignees
No one assigned
Labels
maintenance PR with maintance changes, qt Relates to qt tests Something related to our tests
Projects
None yet
Milestone
0.6.3
Development

Successfully merging this pull request may close these issues.
5 participants
@Czaki @willingc @jni @TimMonko @brisvag
0