Skip to content

Conversation

@AlexEMG
Copy link
Member

@AlexEMG AlexEMG commented May 11, 2023

No description provided.

@AlexEMG AlexEMG requested a review from jeylau May 11, 2023 08:56
Comment on lines 16 to 19
for fps, duration, n_to_pick, start, end, index, valid_frame_indices in [
(32, 10, 10, 0, 1, None, list(range(0, 320))), # 320 frames at 32 fps -> 10 seconds
(16, 100, 50, 0, 1, frame_index_1, frame_index_1), # 1600 frames at 16 fps -> 100 seconds
(16, 100, 5, 0.25, 0.3, frame_index_2, valid_frame_index_2), # 1600 frames at 16 fps -> 100 seconds
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these multiple sets of parameters be moved outside the func in @pytest.mark.parametrize()?; e.g., as in

@pytest.mark.parametrize("shrink, crop", [(1, False), (1, True), (2, False), (2, True)])

When a unit test fails, this makes debugging easier.

Comment on lines 49 to 52
for fps, nframes, n_to_pick, start, end, index, valid_frame_indices in [
(32, 320, 10, 0, 1, None, list(range(0, 320))), # 320 frames at 32 fps -> 10 seconds
(16, 1600, 50, 0, 1, frame_index_1, frame_index_1), # 1600 frames at 16 fps -> 100 seconds
(16, 1600, 5, 0.25, 0.3, frame_index_2, valid_frame_index_2), # 1600 frames at 16 fps -> 100 seconds
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

Comment on lines 139 to 142
for multianimal, bodyparts, multianimalbodyparts, uniquebodyparts in [
(False, all_bodyparts, None, None),
(True, None, all_bodyparts[:3], all_bodyparts[3:]),
]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as below: let's use the parametrize fixture.

@jeylau
Copy link
Contributor

jeylau commented May 11, 2023

Awesome, just left minor comments.

Copy link
Member Author

@AlexEMG AlexEMG left a comment

Choose a reason for hiding this comment

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

Great!

@AlexEMG AlexEMG merged commit c2469ab into DeepLabCut:main May 12, 2023
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.

3 participants