Skip to content
This repository has been archived by the owner on Mar 15, 2023. It is now read-only.

Restore movie data chunk iterator #364

Closed
wants to merge 14 commits into from

Conversation

Saksham20
Copy link
Contributor

@Saksham20 Saksham20 commented Jan 10, 2022

Motivation

Rebasing #265

Additionally, the user has the options to specify multiple video files to go in the same ImageSeries container only when the external_mode=True. The external_file attribute of the ImageSeries gets the list of video files.

How to test the behavior:

from nwb_conversion_tools import (
    NWBConverter,
    MovieInterface
)
from pynwb import NWBHDF5IO

movie_file_paths = ['file1.avi', 'file2.avi']
nwbfile_path = 'nwbfile.nwb'

class MovieTestNWBConverter(NWBConverter):
    data_interface_classes = dict(Movie=MovieInterface)

source_data = dict(Movie=dict(file_paths= movie_file_paths]))
converter = MovieTestNWBConverter(source_data)
metadata = converter.get_metadata()
metadata.update(
            Behavior=dict(
                Movies=[
                    dict(
                        name="CustomName",
                        description="CustomDescription",
                        unit="CustomUnit",
                        resolution=12.3,
                        comments="CustomComments",
                    ),
                    dict(
                        name="CustomName", # the same name. Thus the imageseries will contain a list of video files under external_file
                        description="CustomDescription",
                        unit="CustomUnit",
                        resolution=12.3,
                        comments="CustomComments",
                    )
                ]
            )
        )

converter.run_conversion(metadata=metadata, nwbfile_path=nwbfile_path, overwrite=True)
with NWBHDF5IO(path=nwbfile_path, mode="r") as io:
    nwbfile = io.read()
    custom_name = metadata["Behavior"]["Movies"][0]["name"]
    external_path_list = nwbfile.acquisition[custom_name].external_path
assert set(external_path_list)==set(movie_file_paths)

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 Saksham20 self-assigned this Jan 10, 2022
@Saksham20
Copy link
Contributor Author

TODO:

  • fix tests

@CodyCBakerPhD
Copy link
Member

CodyCBakerPhD commented Jan 10, 2022

@Saksham20 Could you break this PR up into two separate ones?

  • PR 1: introduce new context class and replace util usage with it
  • PR 2: introduce new iterator using context and modifications to writing procedure in main interface

@codecov
Copy link

codecov bot commented Jan 11, 2022

Codecov Report

Merging #364 (446bcd2) into main (2ca38c6) will decrease coverage by 22.90%.
The diff coverage is 0.58%.

Impacted file tree graph

@@             Coverage Diff             @@
##             main     #364       +/-   ##
===========================================
- Coverage   83.03%   60.13%   -22.91%     
===========================================
  Files          41       41               
  Lines        1998     2107      +109     
===========================================
- Hits         1659     1267      -392     
- Misses        339      840      +501     
Flag Coverage Δ
unittests 60.13% <0.58%> (-22.91%) ⬇️

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 0.00% <0.00%> (-80.00%) ⬇️
...atainterfaces/behavior/movie/moviedatainterface.py 22.33% <1.81%> (-72.06%) ⬇️
...s/datainterfaces/ecephys/blackrock/header_tools.py 28.88% <0.00%> (-68.89%) ⬇️
...datainterfaces/ecephys/intan/intandatainterface.py 31.91% <0.00%> (-68.09%) ⬇️
...ainterfaces/ecephys/neuroscope/neuroscope_utils.py 29.62% <0.00%> (-55.56%) ⬇️
...rfaces/ecephys/neuralynx/neuralynxdatainterface.py 47.05% <0.00%> (-52.95%) ⬇️
...terfaces/ecephys/spikeglx/spikeglxdatainterface.py 43.05% <0.00%> (-50.00%) ⬇️
...rfaces/ecephys/blackrock/blackrockdatainterface.py 41.66% <0.00%> (-50.00%) ⬇️
...datainterfaces/ecephys/axona/axonadatainterface.py 29.03% <0.00%> (-41.94%) ⬇️
...atainterfaces/ecephys/baselfpextractorinterface.py 54.54% <0.00%> (-40.91%) ⬇️
... and 10 more

@Saksham20
Copy link
Contributor Author

Saksham20 commented Jan 17, 2022

@Saksham20 Could you break this PR up into two separate ones?

  • PR 1: introduce new context class and replace util usage with it

#372

@CodyCBakerPhD CodyCBakerPhD marked this pull request as draft January 17, 2022 18:28
@Saksham20 Saksham20 closed this Jan 18, 2022
@CodyCBakerPhD CodyCBakerPhD deleted the restore_movie_data_chunk_iterator branch April 17, 2022 16:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants