Skip to content
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.

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

Make config_file_path optional on DeepLabCutInterface #1031

Merged
merged 15 commits into from
Sep 4, 2024

Conversation

h-mayorquin
Copy link
Collaborator

Needed for the Cohen conversion where the files are (hopefully temporarily) missing. Plus, as described in #1030 we don't really extract any critical information from them.

@h-mayorquin h-mayorquin self-assigned this Aug 26, 2024
@h-mayorquin h-mayorquin marked this pull request as ready for review August 26, 2024 19:43
@CodyCBakerPhD
Copy link
Member

Can you add a small test case with the existing example data really quick?

@h-mayorquin
Copy link
Collaborator Author

Can you add a small test case with the existing example data really quick?

What kind of test are you envisioning? That it does not fail when you pass None instead of a file_path?

@CodyCBakerPhD
Copy link
Member

A showcase of w/e new behavior you are adding in this PR insofar as it is different from the current test cases

@h-mayorquin
Copy link
Collaborator Author

The new behavior is that you don't require to pass a configuration file path. Would the test I mentioned cover it?

@CodyCBakerPhD
Copy link
Member

Possibly - unless there is logic in DLC interface for 'searching' for a nearby config, in which case you'd have to making a tmpdir copy of example data w/o the config file

@h-mayorquin
Copy link
Collaborator Author

Possibly - unless there is logic in DLC interface for 'searching' for a nearby config, in which case you'd have to making a tmpdir copy of example data w/o the config file

There is no such a thing.

@h-mayorquin
Copy link
Collaborator Author

This was a good suggestion @CodyCBakerPhD. I found out another nested use of the configuration file deep inside the call stack of the utils.

@h-mayorquin h-mayorquin mentioned this pull request Aug 30, 2024
@h-mayorquin
Copy link
Collaborator Author

Will need this:
#1047

Comment on lines +385 to +391
expected_pose_estimation_series = ["ind1_leftear", "ind1_rightear", "ind1_snout", "ind1_tailbase"]

expected_pose_estimation_series_are_in_nwb_file = [
pose_estimation in pose_estimation_series_in_nwb for pose_estimation in expected_pose_estimation_series
]

assert all(expected_pose_estimation_series_are_in_nwb_file)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expected_pose_estimation_series = ["ind1_leftear", "ind1_rightear", "ind1_snout", "ind1_tailbase"]
expected_pose_estimation_series_are_in_nwb_file = [
pose_estimation in pose_estimation_series_in_nwb for pose_estimation in expected_pose_estimation_series
]
assert all(expected_pose_estimation_series_are_in_nwb_file)
assert all(
pose_estimation in pose_estimation_series_in_nwb for pose_estimation in ["ind1_leftear", "ind1_rightear", "ind1_snout", "ind1_tailbase"]
)

this would be clearer to me

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a pet peeve of mine that I think makes debuggin easier so I would like to explain myself.

The reason that I like avoid definitions inside of other scopes is that it gets on the way of the standard debuggin workflow. With the current code If the test fails, I just set a debuggin point in line 390 (pdb.set_trace()) and then I can check immediatley what is the value of expected_pose_estimation_series_are_in_nwb_file and think about what failed. With the proposed change I would need to evaluate the expression again to inspect it.

Moreover, before python 3.12 list comprehension is debugged as a separate frame so if I use the automated tools of my IDE I can't be in a frame that has access to both values of interest: the values inside the list comprehension and the context from which is called.

All of that said I think the first definition in line 385 can be eliminated to make this more concise but I like having all the defintions before the assertions/checks so I can debug everything in the same frame.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, I rarely use pdb.set_trace, so I mostly go by readability, but I can see how it could get in the way for you. Feel free to ignore

@h-mayorquin h-mayorquin enabled auto-merge (squash) September 4, 2024 21:15
@h-mayorquin h-mayorquin merged commit 675ec48 into main Sep 4, 2024
35 checks passed
@h-mayorquin h-mayorquin deleted the drop_config_file_requierement_in_dlc branch September 4, 2024 22:46
Copy link

codecov bot commented Sep 4, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 8 lines in your changes missing coverage. Please review.

Project coverage is 90.32%. Comparing base (a9b993a) to head (45c7b68).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...v/datainterfaces/behavior/deeplabcut/_dlc_utils.py 70.37% 8 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1031      +/-   ##
==========================================
- Coverage   90.36%   90.32%   -0.04%     
==========================================
  Files         129      129              
  Lines        7938     7988      +50     
==========================================
+ Hits         7173     7215      +42     
- Misses        765      773       +8     
Flag Coverage Δ
unittests 90.32% <75.00%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ces/behavior/deeplabcut/deeplabcutdatainterface.py 93.18% <100.00%> (+0.68%) ⬆️
...v/datainterfaces/behavior/deeplabcut/_dlc_utils.py 55.15% <70.37%> (-3.24%) ⬇️

... and 1 file with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants