Skip to content

Add unit tests for PoseEstimation widgets - #16

Merged
h-mayorquin merged 13 commits into
mainfrom
copilot/add-unit-tests-pose-estimation
Feb 24, 2026
Merged

Add unit tests for PoseEstimation widgets#16
h-mayorquin merged 13 commits into
mainfrom
copilot/add-unit-tests-pose-estimation

Conversation

Copilot AI commented Feb 17, 2026

Copy link
Copy Markdown
Contributor

Summary: Added Unit Tests for PoseEstimation Widgets

Successfully added comprehensive unit tests for the PoseEstimation widgets, following the existing testing conventions in the repository.

Changes Made

New Test File: tests/test_local_pose_widget.py (19 tests)

  • ✅ TestPoseEstimationDiscovery (3 tests)
    • Test discovering pose estimation from single/multiple cameras
    • Test discovery when videos are also present
  • ✅ TestCameraInfoExtraction (2 tests)
    • Test extracting metadata for single/multiple cameras
  • ✅ TestWidgetCreation (5 tests)
    • Test creating widgets with various configurations
    • Test default camera selection and error handling
  • ✅ TestLazyLoading (2 tests)
    • Test that pose data is loaded lazily when camera is selected
  • ✅ TestKeypointColors (3 tests)
    • Test default colormap, custom colors, and different colormaps
  • ✅ TestErrorHandling (2 tests)
    • Test error handling for missing PoseEstimation data
    • Test error handling for in-memory NWB files
  • ✅ TestVideoNameMapping (2 tests)
    • Test video URL extraction and camera-to-video mapping

Updated Source Files:

  • src/nwb_video_widgets/_utils.py - discover_pose_estimation_cameras now searches all processing modules (not just "pose_estimation"), making widgets compatible with NWB files where pose data is in "behavior" or any other module
  • src/nwb_video_widgets/local_pose_widget.py - Validation uses discover_pose_estimation_cameras result instead of hardcoding the "pose_estimation" module; stores a _pose_containers dict instead of a module reference
  • src/nwb_video_widgets/dandi_pose_widget.py - Same improvements as local_pose_widget.py

Updated Test/Config Files:

  • tests/fixtures/synthetic_nwb.py - Uses MockPoseEstimationInterface.add_to_nwbfile() (NeuroConv's standard approach) for the first camera; additional cameras are added directly to the existing "behavior" module to avoid Device/Skeleton naming conflicts from repeated add_to_nwbfile calls
  • tests/conftest.py - Added 3 new fixtures for pose estimation testing
  • pyproject.toml - Added ndx-pose to main package dependencies; neuroconv added to test dependencies
  • CHANGELOG.md - Added entry for v0.1.6 documenting test additions

Test Results

  • ✅ All 19 new tests pass
  • ✅ All 14 existing unit tests still pass
  • ✅ Total: 33 unit tests passing
  • ✅ Linting passed (ruff)
  • ✅ Follows existing testing conventions
Original prompt

This section details on the original issue you should resolve

<issue_title>Tests for PoseEstimation</issue_title>
<issue_description>I was working on #14, and I noticed that there aren't actually any unit tests for the pose estimation widgets, just for the video ones, so probably it would be good to have those. </issue_description>

<agent_instructions>Please follow the conventions in the existing testing architecture wherever possible. </agent_instructions>

Comments on the Issue (you are @copilot in this section)


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI and others added 2 commits February 17, 2026 21:34
Co-authored-by: pauladkisson <34703136+pauladkisson@users.noreply.github.com>
Co-authored-by: pauladkisson <34703136+pauladkisson@users.noreply.github.com>
Copilot AI changed the title [WIP] Add unit tests for pose estimation widgets Add unit tests for PoseEstimation widgets Feb 17, 2026
Copilot AI requested a review from pauladkisson February 17, 2026 21:37

@pauladkisson pauladkisson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@copilot

Tests are showing the following error.

ImportError while loading conftest '/Users/runner/work/nwb-video-widgets/nwb-video-widgets/tests/conftest.py'.
tests/conftest.py:9: in
from tests.fixtures.synthetic_nwb import (
tests/fixtures/synthetic_nwb.py:6: in
from ndx_pose import PoseEstimation, PoseEstimationSeries
E ModuleNotFoundError: No module named 'ndx_pose'

Could you please add ndxpose to the test dependencies?

Co-authored-by: pauladkisson <34703136+pauladkisson@users.noreply.github.com>

Copilot AI commented Feb 17, 2026

Copy link
Copy Markdown
Contributor Author

@copilot

Tests are showing the following error. ...

Fixed in bc0d5e9. The import error occurred because ndx-pose was not included in the test dependencies. The test fixtures in synthetic_nwb.py use PoseEstimation and PoseEstimationSeries from the ndx-pose package to create synthetic test data. Added ndx-pose to the test dependency group in pyproject.toml.

@pauladkisson pauladkisson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good to me.

@pauladkisson
pauladkisson marked this pull request as ready for review February 17, 2026 21:49
@pauladkisson

Copy link
Copy Markdown
Member

@copilot could you update the change log?

Co-authored-by: pauladkisson <34703136+pauladkisson@users.noreply.github.com>

Copilot AI commented Feb 17, 2026

Copy link
Copy Markdown
Contributor Author

@copilot could you update the change log?

Updated in 44d5125. Added entry to CHANGELOG.md under the Improvements section for v0.1.6.

Comment thread CHANGELOG.md Outdated
@h-mayorquin

Copy link
Copy Markdown
Contributor

@pauladkisson

I would like to hear what do you think

I am thinking that:
TestPoseEstimationDiscovery and TestCameraInfoExtraction are testing internals.

Maybe we should remove them, they will slow down refactoring.

Also,even less sure about it, maybe we should use:
https://github.com/catalystneuro/neuroconv/blob/d5fddbde7d2bb398f48fedaf6daef5cb0ee1d0e6/src/neuroconv/tools/testing/mock_interfaces.py#L736

Instead of fixtures here.

Let me know what do you think.

@pauladkisson

Copy link
Copy Markdown
Member

I am thinking that: TestPoseEstimationDiscovery and TestCameraInfoExtraction are testing internals.

Maybe we should remove them, they will slow down refactoring.

Personally, I feel like there are still useful unit tests even if they're just util functions. But happy to remove them if you insist.

Also,even less sure about it, maybe we should use: https://github.com/catalystneuro/neuroconv/blob/d5fddbde7d2bb398f48fedaf6daef5cb0ee1d0e6/src/neuroconv/tools/testing/mock_interfaces.py#L736

Instead of fixtures here.

Let me know what do you think.

Yeah, using the mock from NeuroConv is probably a good idea, just to avoid duplicate code. I'll implement here.

@pauladkisson

Copy link
Copy Markdown
Member

@copilot, please replace the custom mock pose estimation code with a reference to this mock interface from NeuroConv: https://github.com/catalystneuro/neuroconv/blob/d5fddbde7d2bb398f48fedaf6daef5cb0ee1d0e6/src/neuroconv/tools/testing/mock_interfaces.py#L736

Co-authored-by: pauladkisson <34703136+pauladkisson@users.noreply.github.com>

Copilot AI commented Feb 23, 2026

Copy link
Copy Markdown
Contributor Author

@copilot, please replace the custom mock pose estimation code with a reference to this mock interface from NeuroConv: https://github.com/catalystneuro/neuroconv/blob/d5fddbde7d2bb398...

Done in 4664342. The custom mock pose estimation code in tests/fixtures/synthetic_nwb.py has been replaced with MockPoseEstimationInterface from NeuroConv. Key changes:

  • create_nwbfile_with_pose_estimation and create_nwbfile_with_videos_and_pose now use MockPoseEstimationInterface for data generation (Lissajous trajectory, standard body part names)
  • keypoint_names parameter replaced with num_nodes (default 3)
  • test_raises_for_in_memory_nwbfile now uses create_nwbfile_with_pose_estimation instead of inline manual construction
  • Test dep changed from ndx-pose to neuroconv (which provides ndx-pose transitively)
  • Node names updated in test assertions to match NeuroConv's defaults ("Head", "Neck", "LeftShoulder", etc.)

@pauladkisson pauladkisson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You should be able to use the add_to_nwbfile method of the mock interface.

…PoseEstimation

Co-authored-by: pauladkisson <34703136+pauladkisson@users.noreply.github.com>

Copilot AI commented Feb 23, 2026

Copy link
Copy Markdown
Contributor Author

You should be able to use the add_to_nwbfile method of the mock interface.

Done in a148fa0. The fixtures now use add_to_nwbfile for the first camera (which creates the container as "MockPoseEstimation" in the "behavior" module). For additional cameras in multi-camera fixtures, PoseEstimation containers are added directly to the existing "behavior" module to avoid Device/Skeleton naming conflicts from repeated add_to_nwbfile calls. Updated discover_pose_estimation_cameras to search all processing modules (not just "pose_estimation") so the widget works regardless of which module the data lives in. Also added ndx-pose to the main package dependencies as requested.

@h-mayorquin

Copy link
Copy Markdown
Contributor

Personally, I feel like there are still useful unit tests even if they're just util functions. But happy to remove them if you insist.

Yes, let's remove them.

@pauladkisson

pauladkisson commented Feb 24, 2026

Copy link
Copy Markdown
Member

Also,even less sure about it, maybe we should use: https://github.com/catalystneuro/neuroconv/blob/d5fddbde7d2bb398f48fedaf6daef5cb0ee1d0e6/src/neuroconv/tools/testing/mock_interfaces.py#L736
Instead of fixtures here.
Let me know what do you think.

Yeah, using the mock from NeuroConv is probably a good idea, just to avoid duplicate code. I'll implement here.

Actually, upon looking at the implementation, using the mock interface from NeuroConv significantly complicates things:

  • the NeuroConv mock places pose estimation in the behavior module, so that then this PR depends on Support pose estimation objects anywhere in the nwbfile #17 when I wanted it to be prior to that PR.
  • The NeuroConv mock uses a hard coded name for the camera device, so you can't use it to add multiple pose estimation objects and cameras, which the current testing requires.

So I think I would prefer to just keep them independent for now, and maybe we can streamline things down the road.

@pauladkisson

Copy link
Copy Markdown
Member

Yes, let's remove them.

Done!

@pauladkisson

Copy link
Copy Markdown
Member

@h-mayorquin, this is ready for review

@h-mayorquin

Copy link
Copy Markdown
Contributor

Ok, thanks for trying, things to improve on the neuroconv mock for sure.

@h-mayorquin
h-mayorquin merged commit a225eab into main Feb 24, 2026
10 checks passed
@h-mayorquin
h-mayorquin deleted the copilot/add-unit-tests-pose-estimation branch February 24, 2026 16:09
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.

Tests for PoseEstimation

3 participants