EDF time slicing #15

alessandratrapani · 2024-11-12T10:00:12Z

Add an option for slicing edf data based on the time range of miniscope recording.

Get Miniscope session start time (already in datetime format in the metadata.json)
Get Miniscope relative timestamps and convert the first and the last timestamps in datetime format using the session start time
Input these two timestamps in edf interface
Get EDF recording start time in datetime format
Convert EDF relative timestamps in datetime format using EDF recording start time
Slice EDF data inside the time range identified by Miniscope recording start time and stop time in datetime format

…n not class methods

h-mayorquin

Bunch of comments and suggestions but feel free to ignore them. The only critical one to me is to clarify why the nwbfile.session_start_time is not used in the add_to_nwbfile method of the EDFSignals interface.

h-mayorquin · 2024-11-12T14:19:56Z

src/cai_lab_to_nwb/zaki_2024/interfaces/miniscope_imaging_interface.py

@@ -17,6 +18,101 @@
 from neuroconv.utils import DeepDict, dict_deep_update


+def get_session_start_time(folder_path: Union[str, Path]):


Shouldn't we just pass the file path of the json directly instead of passing the folder and then locating it within? What do you think?

I know you are moving these functions only but is something to consider if you think that a small breaking change is worth avoiding the problem of a different file structure in the future.

My first design choice was to pass the file path actually. I don't remember why I changed my mind, probably to avoid to repeat pice of code. But if you also think passing the file_path is the correct way, I will change it back.

h-mayorquin · 2024-11-12T14:20:24Z

src/cai_lab_to_nwb/zaki_2024/interfaces/miniscope_imaging_interface.py

+    return session_start_time
+
+
+def get_miniscope_timestamps(miniscope_folder_path: Union[str, Path]):


Same comment here about the file_path vs folder_path?

h-mayorquin · 2024-11-12T14:24:46Z

src/cai_lab_to_nwb/zaki_2024/interfaces/miniscope_imaging_interface.py

@@ -205,36 +301,6 @@ def __init__(self, folder_path: DirectoryPath):

        self.photon_series_type = "OnePhotonSeries"

-    def _get_session_start_time(self):


Another, equally valid option, is to make these functions static or class methods of the class. This is more of taste and preference and depends on how do you want to organize your code.

h-mayorquin · 2024-11-12T14:26:15Z

src/cai_lab_to_nwb/zaki_2024/interfaces/miniscope_imaging_interface.py

        # Shift when the first timestamp is negative
-        # TODO: Figure why, I copied from miniscope
+        # TODO: Figure why, I copied from Miniscope. Need to shift also session_start_time


Is this an answer to my why? or it was your why? The goal of this is to set the timestamps to start at 0, right?

It was part od the TODO. If we saved the session start time from the metadata, and the timestamps refer to that date. When we shift the timestamps to zero we need to shift back the session start time as well. But I left it there as a comment because I am not sure how to handle this. I will open a follow up PR only on this

h-mayorquin · 2024-11-12T14:26:24Z

src/cai_lab_to_nwb/zaki_2024/interfaces/miniscope_imaging_interface.py

@@ -308,7 +366,7 @@ def add_to_nwbfile(
        imaging_extractor.set_times(times=miniscope_timestamps)

        device_metadata = metadata["Ophys"]["Device"][0]
-        # Cast to string because miniscope extension requires so
+        # Cast to string because Miniscope extension requires so


h-mayorquin · 2024-11-12T14:29:08Z

src/cai_lab_to_nwb/zaki_2024/interfaces/zaki_2024_edf_interface.py

-
-    def __init__(self, file_path: FilePath, verbose: bool = False):
-
+    def __init__(


I think these should be conversion options and not init arguments, as you might want to use run-time information (from other interfaces for example) to determine them.

h-mayorquin · 2024-11-12T14:30:17Z

src/cai_lab_to_nwb/zaki_2024/zaki_2024_convert_session.py



 def get_miniscope_folder_path(folder_path: Union[str, Path]):
+    """


h-mayorquin · 2024-11-12T14:31:40Z

src/cai_lab_to_nwb/zaki_2024/interfaces/miniscope_imaging_interface.py

+def get_session_start_time(folder_path: Union[str, Path]):
+    """
+    Retrieve the session start time from metadata in the specified folder.
+


I also advice to name this get_{something}_start_time now that is not a method of the class or modify the docstring to make this clear.

changed_ in get_recording_start_time

src/cai_lab_to_nwb/zaki_2024/interfaces/zaki_2024_edf_interface.py

h-mayorquin · 2024-11-12T14:40:25Z

src/cai_lab_to_nwb/zaki_2024/interfaces/zaki_2024_edf_interface.py

+        start_datetime_timestamp: datetime = None,
+        stop_datetime_timestamp: datetime = None,
+        verbose: bool = False,
+    ):


Also, if you add them here or in the conversion options (add_to_nwbfile) I think a docstring would be highly beneficial.

…on option

alessandratrapani · 2024-11-14T09:47:13Z

This is working good with your fix catalystneuro/neuroconv#1139
If it's ok with you I would merge this @h-mayorquin

alessandratrapani added 4 commits November 12, 2024 10:41

add option for time slice in edf interface

1d5fdc4

get_original_timestamps and _get_session_start_time as helper functio…

a0533a7

…n not class methods

include edf slicing in conversion script

d9efa22

Merge branch 'main' into add_imaging_timestamps

ab8eafa

alessandratrapani mentioned this pull request Nov 12, 2024

Zaki 2024 Conversion (Cai lab) #1

Open

30 tasks

remove TODO comment

66e600e

alessandratrapani requested a review from h-mayorquin November 12, 2024 10:10

alessandratrapani mentioned this pull request Nov 12, 2024

Add imaging timestamps using new conversion option always_write_timestamps #16

Merged

h-mayorquin reviewed Nov 12, 2024

View reviewed changes

alessandratrapani added 2 commits November 13, 2024 14:50

use file_path instead of folder_path

eaa8e3f

move start_datetime_timestamp and stop_datetime_timestamp to conversi…

50a326a

…on option

h-mayorquin merged commit a4cf9d2 into main Nov 14, 2024

h-mayorquin deleted the edf_slicing branch November 14, 2024 13:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EDF time slicing #15

EDF time slicing #15

alessandratrapani commented Nov 12, 2024

h-mayorquin left a comment

h-mayorquin Nov 12, 2024

alessandratrapani Nov 13, 2024

h-mayorquin Nov 12, 2024

h-mayorquin Nov 12, 2024

h-mayorquin Nov 12, 2024

alessandratrapani Nov 13, 2024

h-mayorquin Nov 12, 2024

h-mayorquin Nov 12, 2024

h-mayorquin Nov 12, 2024

h-mayorquin Nov 12, 2024

alessandratrapani Nov 13, 2024 •

edited

Loading

h-mayorquin Nov 12, 2024

alessandratrapani commented Nov 14, 2024

		@@ -17,6 +18,101 @@
		from neuroconv.utils import DeepDict, dict_deep_update


		def get_session_start_time(folder_path: Union[str, Path]):

		return session_start_time


		def get_miniscope_timestamps(miniscope_folder_path: Union[str, Path]):

		@@ -205,36 +301,6 @@ def __init__(self, folder_path: DirectoryPath):

		self.photon_series_type = "OnePhotonSeries"

		def _get_session_start_time(self):


		def __init__(self, file_path: FilePath, verbose: bool = False):

		def __init__(



		def get_miniscope_folder_path(folder_path: Union[str, Path]):
		"""

EDF time slicing #15

EDF time slicing #15

Conversation

alessandratrapani commented Nov 12, 2024

h-mayorquin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alessandratrapani Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alessandratrapani commented Nov 14, 2024

alessandratrapani Nov 13, 2024 •

edited

Loading