Integrate Movie context manager into DataInterface

[Saksham20]

Motivation

Adding a video capture context manager with various properties. Based on dicussion here

This PR breaks #364 as suggested by @CodyCBakerPhD here

How to test the behavior?

Have setup tests for it in a separate file: https://github.com/catalystneuro/nwb-conversion-tools/blob/952b7f4a530bd8b62ab7185569060d8d1878087d/tests/test_internals/test_movie_interface.py

Once his is merged, I can add another PR for the MovieDataChunkIterator that was implemented at #265

Checklist

• Have you checked our Contributing document?
• Have you ensured the PR description clearly describes the problem and solutions?
• Have you checked to ensure that there aren't other open or previously closed Pull Requests for the same change?
• If this PR fixes an issue, is the first line of the PR description fix #XXX where XXX is the issue number?

[Saksham20]

TODO:

• fix segmentation errors in tests

[Saksham20]

@CodyCBakerPhD you had added this function to add a custom processing module to hold the ImageSeries. But we would need to put the ImageSeries under a NWBContainer, NWBDataInterface. Calling mod.add(ImageSeries) creates an error.
I'm not sure which existing data interface holds a list of ImageSeries type.

[CodyCBakerPhD]

@Saksham20 If you can write a test to show how .add or .add_interface causes an error in the way you describe that would be the best way to understand the issue you're having. See

nwb-conversion-tools/tests/test_internals/test_interfaces.py

Lines 178 to 184 in 335c71f

	assert module_name in nwbfile.modules
	nwbfile = converter.run_conversion(
	metadata=metadata,
	save_to_file=False,
	conversion_options=dict(Movie=dict(module_name=module_name, module_description=module_description)),
	)
	assert module_name in nwbfile.modules and nwbfile.modules[module_name].description == module_description

for the current test of this functionality

[CodyCBakerPhD]

@Saksham20 Mark as ready for review once tests are passing; I'm also kind of curious what is so severe here that we get segmentation faults?

[CodyCBakerPhD]

@Saksham20 Can you point me to the logic in the code responsible for handling stub_test=True when external_mode=False? The one that shortens the number of frames loaded.

Also if you can add some tests that ensure such a method works correctly.

[Saksham20]

@Saksham20 Can you point me to the logic in the code responsible for handling stub_test=True when external_mode=False? The one that shortens the number of frames loaded.

Also if you can add some tests that ensure such a method works correctly.

I missed adding the logic to change the frame_count of the video capture object when stub_test is true. bca08f1

[CodyCBakerPhD]

Related - an old issue from the first version of this interface: NeurodataWithoutBorders/pynwb#1318

Can you look into it and see if it can be closed? Or if it needs more action propagated upward?

Can we set multiple starting_times when external_mode=True and more than one file path is being specified?

[Saksham20]

Related - an old issue from the first version of this interface: NeurodataWithoutBorders/pynwb#1318
Can you look into it and see if it can be closed? Or if it needs more action propagated upward?

Can we set multiple starting_times when external_mode=True and more than one file path is being specified?

Yeah thanks for bringing that up. I have though of that too, I've recreated the issue with a simpler code snippet NeurodataWithoutBorders/pynwb#1433

[CodyCBakerPhD]

@Saksham20 Just need a test that explicitly verifies the data size using stub_test=False and external_mode=False is exactly 10 frames (for a video > 10 frames).

Then should be good to go.

[Saksham20]

@Saksham20 Just need a test that explicitly verifies the data size using stub_test=False and external_mode=False is exactly 10 frames (for a video > 10 frames).

Oh yeah, missed that. Done bd67a7b

[codecov]

Codecov Report

Merging #372 (bd67a7b) into main (2d9c90e) will increase coverage by 0.03%.
The diff coverage is 98.36%.

❗ Current head bd67a7b differs from pull request most recent head c0df303. Consider uploading reports for the commit c0df303 to get more accurate results

@@            Coverage Diff             @@
##             main     #372      +/-   ##
==========================================
+ Coverage   87.56%   87.60%   +0.03%     
==========================================
  Files          42       42              
  Lines        2381     2371      -10     
==========================================
- Hits         2085     2077       -8     
+ Misses        296      294       -2     

Flag	Coverage Δ
unittests	87.60% <98.36%> (+0.03%)	⬆️

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

Impacted Files	Coverage Δ
...tools/datainterfaces/behavior/movie/movie_utils.py	95.04% <ø> (+4.06%)	⬆️
...atainterfaces/behavior/movie/moviedatainterface.py	97.02% <98.36%> (+2.58%)	⬆️
nwb_conversion_tools/nwbconverter.py	88.88% <0.00%> (-6.18%)	⬇️
nwb_conversion_tools/utils/nwbfile_tools.py	92.85% <0.00%> (-3.58%)	⬇️