add name parameter to the DeepLabCutInterface

[vigji]

This PR should fix the issue described in #915.

It consists in the small addition of a name argument to the DeepLabCutInterface that allows users to specify names different from the nwb-pose default PoseEstimation (https://github.com/rly/ndx-pose/blob/a847ad4be75e60ef9e413b8cbfc99c616fc9fd05/spec/ndx-pose.extensions.yaml#L71).

To work, the call to dlc2nwb. _write_pes_to_nwbfile() should accept additional arguments, as described in DeepLabCut/DLC2NWB#24. At the moment, and until dlc2nwb people give feedback on that issue, this functionality would require a different fork of dlc2nwb: https://github.com/vigji/DLC2NWB.

[vigji]

Reasonable suggestion! Let me know if there is anything else I should do

[CodyCBakerPhD]

Last thing to do would be to add a new test case that uses this new feature after the current test case: https://github.com/catalystneuro/neuroconv/blob/main/tests/test_on_data/test_behavior_interfaces.py#L317

Which would also require setting developer dependencies (in the form of git+https://github.com/vigji/dlc2nwb.git@main to https://github.com/catalystneuro/neuroconv/blob/main/src/neuroconv/datainterfaces/behavior/deeplabcut/requirements.txt) to install the version of the DLC tools that allow for it so we can make sure both packages work well together

[vigji]

Last thing to do would be to add a new test case that uses this new feature after the current test case: https://github.com/catalystneuro/neuroconv/blob/main/tests/test_on_data/test_behavior_interfaces.py#L317

Hope I did this correctly

Which would also require setting developer dependencies (in the form of git+https://github.com/vigji/dlc2nwb.git@main to https://github.com/catalystneuro/neuroconv/blob/main/src/neuroconv/datainterfaces/behavior/deeplabcut/requirements.txt) to install the version of the DLC tools that allow for it so we can make sure both packages work well together

I changed this in requirements-testing.txt, is this correct?

[CodyCBakerPhD]

Test looks good! Will need to wait a bit to merge upstream before getting this through

Thanks for all the work

[vigji]

Perfect, let me know if there's anything more I need to take care of, and thank you :)

[vigji]

After having tested this in parallel with the VideoInterface for the conversion project, I have to say I am slightly annoyed by the different approaches to solve the same issue: an argument to the object instantiation there, an optional parameter to the conversion function call here. Should I change this to be consistent with VideoInterface? What would be your approach? Am I missing something?

[CodyCBakerPhD]

The usual excuse for that is that various decisions were made for the sake of maintaining back-compatibility; the video interfaces is one of our oldest and most used interfaces after all (since it is so generic) - so if we were to change something fundamental about it, it might cause problems for a number of conversion pipelines

That said, we do try to standardize these things with slow deprecation cycles every now and again. Enhancing the video interfaces is something we've wanted to do for a while now but no one has had the time to implement all improvements

[vigji]

Perfectly reasonable! So the strategy used here for the DeeplacutInterface is the way to go, correct?

[CodyCBakerPhD]

Yes, something like a container name seems to me something more relevant as a conversion option (this is also the case in ophys/ecephys modules here)

Just waiting on the upstream repo to respond to PR and we can get this through. I'll inject myself there soon otherwise just to get this done

[CodyCBakerPhD]

@vigji Sorry this took so long; we're just taking matters into our own hands at this point

There should be a box somewhere on this PR about giving reviewers permission to push code or something like that - if you check that box I'd be happy to make the required changes to get this through

[vigji]

Great! thanks! Could not find such button but I should have applied all changes.

[CodyCBakerPhD]

@vigji There is a remaining merge conflict which must be resolved via the CLI (be sure to try to merge port_dlc2nwb on our end)

[CodyCBakerPhD]

Thanks again @vigji !