Uniform clip sampling + multi-clip sampling function by themattinthehatt · Pull Request #57 · neuroinformatics-unit/poseinterface

themattinthehatt · 2026-05-29T19:30:22Z

Description

What is this PR

Bug fix
Addition of a new feature
Other

Why is this PR needed?
Current repo only has a single clip selection strategy (manual).

What does this PR do?
Adds a uniform clip sampling strategy option, and updates the CLI to take sampling strategy as input.

References

Builds on #39.

How has this PR been tested?

Added new unit tests, checked all tests are passing locally before making PR.

Is this a breaking change?

Yes; the previous clip extraction entry point was extract-clip; this has been updated to extract-clips to work with multiple clips; in addition, the command line args changed to allow for multiple clip sampling strategies.

Does this PR require an update to the documentation?

Minor update renaming the clip extraction entrypoint from extract-clip to extract-clips.

Checklist:

The code has been tested locally
Tests have been added to cover all new functionality
The documentation has been updated to reflect any changes
The code has been formatted with pre-commit

sfmig

Hi @themattinthehatt, thanks for this!

I made a few suggestions: two of them are two options to try to keep the kwargs within static type checking; the other one is a decoupling suggestion that I think can help simplify tests and has other benefits too.

I think the decoupled approach would be my preferred one in terms of clarity, but it may be a personal preference too. I leave you all the options, let me know what you think. I requested changes in case you want to go for any one of them, I'd be happy to have another look. I haven't checked the tests in detail for the same reason.

themattinthehatt · 2026-06-11T16:48:07Z

@sfmig thanks for the great feedback! I incorporated many of you suggestions. I also imagine at some point we'll build a proper CLI module and move wrapper()/main()/parse_args() out of this module. Is that what you had in mind, or do you want to keep all of this functionality in one place?

I have one test failing: https://github.com/neuroinformatics-unit/poseinterface/actions/runs/27362615831/job/80853717159?pr=57. Do you know why this is the case? I didn't touch this module on my side, is this related to some update in movement?

niksirbi · 2026-06-11T17:51:54Z

I have one test failing: neuroinformatics-unit/poseinterface/actions/runs/27362615831/job/80853717159?pr=57. Do you know why this is the case? I didn't touch this module on my side, is this related to some update in movement?

Ah yeah, that's because of the breaking change of dimension names in movement v0.17.0. To unblock the ongoing PRs, we could temporarily pin movement to v0.16.0 here and then we can follow up with a separate PR bringing the codebase up-to-speed with movement v0.17.0.

niksirbi · 2026-06-11T18:24:01Z

@themattinthehatt, I've quickly fixed the movement issue (it was an easy fix).

If you give #59 a quick approval, we can merge that first and then rebase your 2 PRs to main (that would be the 'cleaner' fix).

…TING guide

…lips when shared (both are public API). Small suggested edit for extract_clips_uniform one-line summary

* Use explicit kwargs to avoid confusion where possible. Parametrise `test_validate_clip_request_valid`. * Include `test_parse_args_invalid_sampling` as a parametrised case of `test_parse_args_missing_required`, and rename to `test_parse_args_invalid_inputs`. * Define variables for inputs to tested functions, to have a single point of data entry (nice if it ever gets modified) * Spell out the error message for clarity in `test_main_uniform_missing_num_clips` and `test_main_manual_missing_start_frames`

…ing test assertions edits

sfmig

Hi @themattinthehatt, thanks for having a second look!

I pushed some small edits myself, hope that is ok. I contained them in separate commits in case you'd like to revert any/all of them. We can discuss any of these in the meeting tomorrow.

The smaller changes are:

Moving the guidelines on contributing sampling strategies from the clips.py module description to the contributing.md (this will make it more findable I think). [commit]
Make the order of the input arguments in extract_single_clip follow extract_clips when arguments are shared (mainly for clarity because both are public API). [commit]
Fix tests accordingly, and other small edits to tests: parametrise one test, include one tests into the parametrisation of the previous one, etc ([commit])

A more substantial change is about _uniform_start_frames. Right now, the duration parameter is only used to check if a single clip fits in the whole video. I found this a bit surprising. I think I would either:

drop duration and say clearly in the docstring that clips are not guaranteed to be non-overlapping or fully in-bounds (i.e. the last clip may be shorter), OR
keep duration, but use it to spread the start frames uniformly over the range [0, n_frames - duration] so we guarantee clips are fully in-bounds and are of same duration (even if they can still have overlaps between them).

I had a go at that second option in this commit in case you want to have a look. It includes edits in the tests too (but again we can revert it, if you had something else in mind).

sfmig · 2026-06-17T16:51:08Z

 @patch("poseinterface.clips.sio.save_video")
 @patch("poseinterface.clips.sio.load_video")
-def test_extract_clip_clamped(
+def test_extract_single_clip_no_labels(


good catch!

sfmig · 2026-06-17T17:00:43Z

+# ---------------------------------------------------------------------------
+
+
+def test_validate_clip_request_valid():


This repeated call in the test can be parametrized

sfmig · 2026-06-17T17:32:54Z

+@patch(
+    "poseinterface.clips.extract_clips", side_effect=ValueError("bad frame")
+)
+def test_main_propagates_value_error(mock_extract_single_clips):


Suggested change

def test_main_propagates_value_error(mock_extract_single_clips):

def test_main_propagates_value_error(mock_extract_clips):

sfmig · 2026-06-17T17:33:03Z

+
+
+@patch("poseinterface.clips.extract_clips")
+def test_main_manual(mock_extract_single_clips):


Suggested change

def test_main_manual(mock_extract_single_clips):

def test_main_manual(mock_extract_clips):

sfmig · 2026-06-17T17:33:25Z

-@patch("poseinterface.clips.extract_clip")
-def test_main(mock_extract_clip):
-    """Test main calls extract_clip with parsed arguments."""
+def test_parse_args_invalid_sampling():


This test could be included in the parametrisation of the one just above

themattinthehatt added 5 commits May 29, 2026 14:58

extract_clips and uniform sampling

4131d87

extract tests

6284c23

multi-clip extraction with CLI

43a0039

cli tests

299145a

update docs

a33a6b5

themattinthehatt requested a review from sfmig May 29, 2026 19:30

sfmig requested changes Jun 8, 2026

View reviewed changes

Comment thread poseinterface/clips.py Outdated

Comment thread poseinterface/clips.py Outdated

Comment thread poseinterface/clips.py Outdated

Comment thread poseinterface/clips.py

Comment thread poseinterface/clips.py Outdated

themattinthehatt added 3 commits June 11, 2026 12:20

explicit kwargs in extract_clips

93aa58c

refactor clip extraction logic

d05174b

increase test coverage

f034db7

extract_clip->extract_single_clip

4c34cb8

themattinthehatt mentioned this pull request Jun 11, 2026

Add Lightning Pose conversion example #56

Merged

7 tasks

update api

de93ba2

themattinthehatt added 2 commits June 11, 2026 14:06

fix tests

06a92b9

pin movement for tests

97601bf

niksirbi mentioned this pull request Jun 11, 2026

Pin movement to >0.17.0 and rename dims to singular #59

Merged

7 tasks

Merge branch 'main' into clipsampling

5b91a15

themattinthehatt requested a review from sfmig June 12, 2026 13:17

sfmig added 7 commits June 17, 2026 19:36

Add extract_clips_uniform to the API docs

4968a68

Move guidelines for Contributing clip sampling strategies to CONTRIBU…

3d01916

…TING guide

Make order of input arguments in extract_single_clip follow extract_c…

a8c7006

…lips when shared (both are public API). Small suggested edit for extract_clips_uniform one-line summary

Suggested use of duration in _uniform_start_frames and correspond…

184506d

…ing test assertions edits

Merge branch 'main' into clipsampling

af5e83c

Rename extract_clip --> extract_single_clip

bf5c51a

sfmig approved these changes Jun 17, 2026

View reviewed changes

		# ---------------------------------------------------------------------------


		def test_validate_clip_request_valid():

	def test_main_propagates_value_error(mock_extract_single_clips):
	def test_main_propagates_value_error(mock_extract_clips):



		@patch("poseinterface.clips.extract_clips")
		def test_main_manual(mock_extract_single_clips):

	def test_main_manual(mock_extract_single_clips):
	def test_main_manual(mock_extract_clips):

Uh oh!

Conversation

themattinthehatt commented May 29, 2026

Description

References

How has this PR been tested?

Is this a breaking change?

Does this PR require an update to the documentation?

Checklist:

Uh oh!

sfmig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

themattinthehatt commented Jun 11, 2026

Uh oh!

niksirbi commented Jun 11, 2026

Uh oh!

niksirbi commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sfmig left a comment

Choose a reason for hiding this comment

Uh oh!

sfmig Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

sfmig Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

sfmig Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

sfmig Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

sfmig Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

niksirbi commented Jun 11, 2026 •

edited

Loading