New pixel time offsets and cycle times #58

alessandrofelder · 2020-07-02T09:37:51Z

This PR will handle differences between LabView versions in storing/computing pixel time offsets and cycle time.

this is a move towards allowing to account for different ways of storing/calculating cycle time and pixel time offsets from LabView version to LabView version.

flake8 didn't like that empty line

assertions and assigning of pixel time offsets still needs to be adapted to newer 231 version. also, pandas reading the roi data as uint16 causes rounding errors, which mess with using z_start as key for the ROI plane

ageorgou

Some small comments. I haven't looked at the code for the computation in the newer version yet but it's looking good so far!

ageorgou · 2020-07-09T17:40:19Z

src/silverlabnwb/nwb_file.py

+        timings = LabViewTimingsPre2018(file_path)
+        self.cycle_relative_times = timings.pixel_time_offsets
+        self.cycle_time = timings.cycle_time
+
+    def read_cycle_relative_times_v231(self, file_path, roi_path):
+        timings = LabViewTimings231(file_path,
+                                    roi_path=roi_path,
+                                    n_cycles_per_trial=self.imaging_info.cycles_per_trial,
+                                    n_trials=len(self.trial_times))
+        self.cycle_relative_times = timings.pixel_time_offsets
+        self.cycle_time = timings.cycle_time


Since these are quite similar, it may be better to have a single method read_cycle_relative_times which takes a path rather than a file name:

def read_cycle_relative_times(self, path): file_path = ... if version is old: timings = LabViewTimingsPre2018(file_path) else if version is new: roi_path = ... timings = LabViewTimings231(file_path, roi_path) self.cycle_relative_times = ...

Or we could even have the Timings classes take a path in their constructor, but that makes them less general (and makes testing harder).

We should also update the docstring to replace the specific filename with something more descriptive that applies to both versions.

ageorgou · 2020-07-09T17:48:05Z

src/silverlabnwb/timings.py

+class LabViewTimingsPre2018(LabViewTimings):
+
+    def __init__(self, relative_times_path):
+        self._read_relative_times_file(relative_times_path)


This (same for the other subclass) should call the base LabViewTimings constructor using super(), to ensure the object is properly initialised. In this case it doesn't really matter because the base class constructor only initialises some attributes which will be overwritten later!

In fact, since they both call _read_relative_times_file as the first step, that should maybe go into the base class __init__ that both subclasses call - unless we think that other subclasses would not.

ageorgou · 2020-07-09T17:49:37Z

src/silverlabnwb/timings.py

+
+    def _read_relative_times_file(self, file_path):
+        raw_data = pd.read_csv(file_path, names=('RelativeTime', 'CycleTime'),
+                               sep='\t', dtype=np.float64) / 1e6


Maybe a note here that we're converting to seconds for consistency? It's in the docstring of the NWBFile's method but worth moving/adding here too, I think.

ageorgou · 2020-07-09T17:53:50Z

src/silverlabnwb/timings.py

+        # in 3D, in which case the ==0 assertion may become imprecise and we would need a tolerance to compare with a
+        # double value.


Good to have the future changes in mind! We could use math.isclose or np.isclose for checking against 0 in that case.

ageorgou · 2020-07-09T17:54:59Z

tests/test_timings.py

+    # the synthetic file has
+    # 2 trials, each of which has 3 cycles and 4 rois a 5 lines
+    # and a few zero lines as may be expected "in the wild"
+    # first cycle ofs trial 1 and trial 2 take 1300.4 and 1200.4 nanoseconds, respectively


Suggested change

# first cycle ofs trial 1 and trial 2 take 1300.4 and 1200.4 nanoseconds, respectively

# first cycle of trial 1 and trial 2 take 1300.4 and 1200.4 nanoseconds, respectively

add data structure that stores pixel time offsets by ROI to timings class write pixel time offsets for first cycle in new labview

remove old datastructure completely, rename some indices

addresses initial review comments - hide versioning inside read_cycle_relative_times - call timings base class constructor - add comment - typo

ageorgou

Looking good. What's left is recording the times for the other cycles in the NWBFile?

It may make sense to move the computation out of add_rois and into the LabViewTimings subclasses. I was thinking they would have a get_roi_offsets(roi_number) method or something similar that the NwbFile would call. Then all the row calculations etc could happen inside the new class, allowing for less branching in the main file. That should also let us write tests more easily (e.g. get all the times for a ROI).

Or would the timings objects be missing something they need to compute the offsets themselves? I suppose they need the dwell time... we could pass that in, or could return the offset of each row in the ROI rather than the whole rectangle.

ageorgou · 2020-07-17T10:46:16Z

src/silverlabnwb/nwb_file.py

@@ -591,24 +586,34 @@ def add_stimulus(self):
            self.nwb_file.add_stimulus(TimeSeries(**attrs))
        self._write()

-    def read_cycle_relative_times_pre2018(self, file_path):
+    def read_cycle_relative_times(self, folder_path):
        """Read the 'Single cycle relative times.txt' file and store the values in memory.


We should probably update this to be more generic. Something like: (roughly)

"""Read the files containing relative times and store the values in memory.

Depending on the version of software used, the file is named either (...) or (...), and in newer versions this can also require reading the ROI.dat file.
Note that while the file has...
"""

ageorgou · 2020-07-17T10:47:18Z

src/silverlabnwb/nwb_file.py

+            self.cycle_relative_times = timings.pixel_time_offsets
+            self.cycle_time = timings.cycle_time


These can go outside the if since they're shared in both branches.
(I now see that one of the fields is named differently (pixel_time_offsets_*by_roi*), but maybe that's by accident?)

yea, you're right - that is a mess-up by yours truly :)

ageorgou · 2020-07-17T10:55:32Z

src/silverlabnwb/nwb_file.py

                                      pixel_time_offsets=pixel_time_offsets,
+                                      dimensions=dimensions,


This confused me, but it's just changing the order of the arguments, right? Nothing else is going on?

pixel_time_offsets will be moved from segmentation to roi data this allows more flexibility to vary the dimensions and the shape of the pixel_time_offsets between ROIs

ageorgou · 2020-07-21T16:54:41Z

src/silverlabnwb/nwb_file.py

+                    all_attrs = dict(ts_attrs)
+                    all_attrs.update(data_attrs)
+                    roi_with_offsets = ROISeriesWithPixelTimeOffsets(name=ts_name,
+                                                                     data=data,
+                                                                     pixel_time_offsets=pixel_time_offsets,
+                                                                     rate=1.0/self.cycle_time,
+                                                                     **all_attrs)
+                    self.nwb_file.add_acquisition(roi_with_offsets)


This can possibly use self._add_time_series_data(kind=ROISeriesWithPixelTimeOffsets, ...)?

ageorgou · 2020-07-21T16:55:57Z

src/silverlabnwb/generate_extended_schema.py

+                                                          'relative to the starting time/timestamp of a '
+                                                          'ROISeriesWithPixelTimeOffsets.',


Is it relative to the starting time (= time of the cycle?) or to the start of the trial?

moved within-line pixel time offset computation inside timings class making the interface more unified among subclasses

consistent API across versions unifies handling of pixel_time_offsets adapt roi extension to shape of pre2018 versions pixel_time_offsets completely removed from ImageSegmentation in all versions to form part of ROI image data.

Before this, there was an issue when planes had more than one ROI. This fixes it for now but we should find a better solution.

The newest release (2.0.0) adds some attributes and allows slightly different types, resulting in changes to the signatures. pynwb is still using the older versions too, even in the most recent release (1.3.3).

ageorgou

This looks basically done, but with some (generally small) comments below.

Some improvements can wait for another PR. I'll leave a comment about that.

ageorgou · 2020-07-24T13:56:38Z

src/silverlabnwb/generate_extended_schema.py

+                                                          'relative to the starting time/timestamp of a '
+                                                          'ROISeriesWithPixelTimeOffsets.',


This should be changed to "starting time of a trial" (can run a quick check to confirm first).

ageorgou · 2020-07-24T13:57:28Z

src/silverlabnwb/generate_extended_schema.py

+                                                      name='pixel_time_offsets',
+                                                      shape=[(None, None), (None, None, None), (None, None, None, None)],
+                                                      neurodata_type_def='PixelTimeOffsets')
+    silverlab_roi_image_specs = NWBGroupSpec(doc='documentation of this class goes here',


We should put something there :)

ageorgou · 2020-07-24T14:00:43Z

src/silverlabnwb/nwb_file.py

@@ -583,15 +586,37 @@ def add_stimulus(self):
            self.nwb_file.add_stimulus(TimeSeries(**attrs))
        self._write()

-    def read_cycle_relative_times(self, file_path):
+    def read_cycle_relative_times(self, folder_path):
        """Read the 'Single cycle relative times.txt' file and store the values in memory.


See the previous comment about this.

ageorgou · 2020-07-24T14:04:12Z

src/silverlabnwb/nwb_file.py

+            self.raw_pixel_time_offsets = timings.pixel_time_offsets
+            self.cycle_time = timings.cycle_time


These lines should go outside the if as they're shared.

ageorgou · 2020-07-24T14:06:00Z

src/silverlabnwb/nwb_file.py

+        else:
+            raise ValueError('Unsupported LabView version for timings {}.'.format(self.labview_version))


This is fine for now, but if you want to avoid it, we can add a method to the (abstract) Timings class which creates an object of the appropriate type based on the version we pass it (similar to how the abstract Header class works, if I remember right). If we pass the wrong version (or the wrong arguments for that version), we can throw the error there.

This can wait until later, however.

ageorgou · 2020-07-24T14:44:15Z

tests/data/synthetic v231 ROI.dat

+1	3	133200	888	70	100	0	76	105	0	0	0	5	512	1	45.1	0		
+2	3	133200	888	80	200	1	86	205	1	0	0	5	512	1	45.2	0		
+3	3	133200	888	90	300	2	96	305	2	0	0	5	512	1	45.3	0		
+4	3	133200	888	100	400	3	106	405	3	0	0	5	512	1	45.4	0		


Same comment as in the other test file :)

ageorgou · 2020-07-24T14:46:28Z

tests/data/synthetic pre2018 Single cycle relative times.txt

@@ -0,0 +1,8 @@
+400.0	12345.000


Hm, does this work even without the header line? Interesting!

Yes, none of the pre2018 data that I have seen have headers here. The column designation is inferred from the order and added in the code.

ageorgou · 2020-07-24T14:54:00Z

tests/test_timings.py

+
+
+def test_pixel_time_offsets_for_roi_v231(synthetic_timings_v231):
+    roi_0_offsets = synthetic_timings_v231.pixel_time_offsets[0]


We should check the other ROI too; this is the recent error that we had to work around in 661aca6.

ageorgou · 2020-07-24T14:55:27Z

tests/test_timings.py

+    assert roi_0_offsets.shape == expected_shape
+    assert roi_0_offsets[0][0][0] == expected_first_cycle_first_row_offset
+    assert roi_0_offsets[0][4][0] == expected_first_cycle_last_row_offset
+    assert roi_0_offsets[5][0][0] == expected_last_cycle_first_row_offset


We're already checking the size earlier, but if we want to be clearer that we're checking the last cycle, I'd put roi_0_offsets[-1][0][0].

ageorgou · 2020-07-24T14:56:19Z

tests/test_timings.py

+
+
+def test_pixel_time_offsets_for_roi_pre2018(synthetic_timings_pre2018):
+    roi_0_offsets = synthetic_timings_pre2018.pixel_time_offsets[0]


As in the other test, we should check the other ROIs too. Perhaps the first and last line of each?

ageorgou · 2020-07-24T15:08:51Z

Once the above changes are made, it would be good to compare with the existing analysis code if possible.

There are other possible improvements but better left for another round, since this is already quite a lot! For example:

Try compressing the pixel data, either in the same way as the manifold and image data, or through a method of the new class (ideally, only store the row offsets and the dwell time, and have a method that computes the full array).
Use a "factory method" for creating an LabViewTimings of the right subclass.
How we check for pointing mode in LabViewTimings
The interface of LabViewTimings (is a dict acceptable, or should we have a method for getting the values for a particular ROI?) This may particularly help with...
How we refer to ROIs, especially when we retrieve the timings for them (improve the temporary solution from 661aca6)
How we build up the offsets array in LabViewTimings231 (as commented in the review)
The location of the test input files: now that we have a lot of them, should we put them in separate folders?
AF Edit: as mentioned in one of the @ageorgou 's comments above, it would also be good to avoid a loop over all cycles in timings.py if possible. Noting here so everything is in one place.

improve docstring move common code outside if-else clauses consistent passing of arguments

add docstring to abstract timings class make comment more precise clarify confusing variable name remove unnecessary comment (see #60)

required because of modified docstrings in silverlab specs

Also includes changes due to #58 (moving pixel time offsets). Otherwise the only changes are to the spec and the new LabVIEW version, plus the datatype of the BodyCam image series.

alessandrofelder force-pushed the new-timings branch from 7a9aae4 to 4d23769 Compare July 6, 2020 15:14

alessandrofelder added 8 commits July 6, 2020 18:08

introduce member variable for cycle_time

05eaf78

this is a move towards allowing to account for different ways of storing/calculating cycle time and pixel time offsets from LabView version to LabView version.

fix cycle_time bug, switch to cycle_relative_times as one column only

253472e

fix indentation

d533b20

basic timing class structure

0427d0b

refactor timings class

afa4c1b

add cycle time estimation

a934462

add comment about numerical error

f2d6bf0

add synthetic test data and basic v231 test for timings

0f47464

alessandrofelder force-pushed the new-timings branch from 4d23769 to 0f47464 Compare July 8, 2020 13:15

alessandrofelder added 6 commits July 8, 2020 14:29

main function 'integration test' and printouts no longer needed

37155f4

remove empty line

94ec186

flake8 didn't like that empty line

fix isort

5c97cef

revert overzealous introduction of v231 timings

dd9f859

wip:distinguish timings by Labview version

14afaca

assertions and assigning of pixel time offsets still needs to be adapted to newer 231 version. also, pandas reading the roi data as uint16 causes rounding errors, which mess with using z_start as key for the ROI plane

fix isort

59be70f

ageorgou reviewed Jul 9, 2020

View reviewed changes

alessandrofelder added 7 commits July 10, 2020 15:47

convert zstart and zstop to float64

39bae1b

wip: add new timings functionality

f4d4f06

add data structure that stores pixel time offsets by ROI to timings class write pixel time offsets for first cycle in new labview

fix linting

d46aa79

merge new pixel_time_offset structures into one, more thorough testing

81197a5

calculate cycle time from new datastructure

5233438

remove old datastructure completely, rename some indices

minor improvements

5b250b9

addresses initial review comments - hide versioning inside read_cycle_relative_times - call timings base class constructor - add comment - typo

improve naming of expected variables in tests

d93430f

ageorgou reviewed Jul 17, 2020

View reviewed changes

alessandrofelder added 2 commits July 21, 2020 17:15

wip: add custom class for pixel offsets and roi image data

8f1712e

wip: use/adapt custom class to hold pixel_time_offsets

80aad1d

pixel_time_offsets will be moved from segmentation to roi data this allows more flexibility to vary the dimensions and the shape of the pixel_time_offsets between ROIs

ageorgou reviewed Jul 21, 2020

View reviewed changes

alessandrofelder added 2 commits July 22, 2020 11:31

refactor timings class

59695b9

moved within-line pixel time offset computation inside timings class making the interface more unified among subclasses

adapt nwb_file.py to cleaner timings class

e079198

consistent API across versions unifies handling of pixel_time_offsets adapt roi extension to shape of pre2018 versions pixel_time_offsets completely removed from ImageSegmentation in all versions to form part of ROI image data.

alessandrofelder and others added 7 commits July 22, 2020 14:59

quickfix timings for pointing modes

777886c

regenerated signatures

58f67df

Get the times for the correct ROI each time

661aca6

Before this, there was an issue when planes had more than one ROI. This fixes it for now but we should find a better solution.

Pin version of hdmf to avoid signature changes

3d2098c

The newest release (2.0.0) adds some attributes and allows slightly different types, resulting in changes to the signatures. pynwb is still using the older versions too, even in the most recent release (1.3.3).

Update signature for metadata test with new schema

0e5860e

add tests for timings pre2018

c2bfd59

add synthetic data for pre2018 tests

755a41c

ageorgou requested changes Jul 24, 2020

View reviewed changes

ageorgou marked this pull request as ready for review July 24, 2020 14:58

alessandrofelder self-assigned this Jul 27, 2020

alessandrofelder added 5 commits July 27, 2020 18:51

tidying of relative times handling in nwb_file

e9f493d

improve docstring move common code outside if-else clauses consistent passing of arguments

tidy timings class

9d9bfab

add docstring to abstract timings class make comment more precise clarify confusing variable name remove unnecessary comment (see #60)

improve/add doc to specification

a947d99

consistent Pixels in ROI for synthetic ROI.dat files

03687f4

test offsets for all ROIs

6e3560e

alessandrofelder requested a review from ageorgou July 28, 2020 10:02

alessandrofelder added 4 commits July 28, 2020 11:38

update signatures

6352b4e

required because of modified docstrings in silverlab specs

reformat file for flake8

7b3f196

more reformatting for flake8

4c4ba74

remove unintended change

1bb05de

ageorgou approved these changes Jul 28, 2020

View reviewed changes

alessandrofelder merged commit 3f8ed12 into new-labview Jul 28, 2020

alessandrofelder deleted the new-timings branch July 28, 2020 11:34

ageorgou added a commit that referenced this pull request Aug 19, 2020

Update signatures for large files

f69d1a9

Also includes changes due to #58 (moving pixel time offsets). Otherwise the only changes are to the spec and the new LabVIEW version, plus the datatype of the BodyCam image series.

ageorgou mentioned this pull request Aug 19, 2020

Record LabVIEW version in metadata #66

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New pixel time offsets and cycle times #58

New pixel time offsets and cycle times #58

alessandrofelder commented Jul 2, 2020

ageorgou left a comment

ageorgou Jul 9, 2020

ageorgou Jul 9, 2020

ageorgou Jul 9, 2020

ageorgou Jul 9, 2020

ageorgou Jul 9, 2020

ageorgou Jul 9, 2020

ageorgou left a comment

ageorgou Jul 17, 2020

ageorgou Jul 17, 2020

alessandrofelder Jul 17, 2020

ageorgou Jul 17, 2020

ageorgou Jul 21, 2020

ageorgou Jul 21, 2020

ageorgou left a comment

ageorgou Jul 24, 2020

ageorgou Jul 24, 2020

ageorgou Jul 24, 2020

ageorgou Jul 24, 2020

ageorgou Jul 24, 2020

ageorgou Jul 24, 2020

ageorgou Jul 24, 2020

alessandrofelder Jul 27, 2020

ageorgou Jul 24, 2020

ageorgou Jul 24, 2020

ageorgou Jul 24, 2020

ageorgou commented Jul 24, 2020 •

edited by alessandrofelder

Loading

		# in 3D, in which case the ==0 assertion may become imprecise and we would need a tolerance to compare with a
		# double value.

	# first cycle ofs trial 1 and trial 2 take 1300.4 and 1200.4 nanoseconds, respectively
	# first cycle of trial 1 and trial 2 take 1300.4 and 1200.4 nanoseconds, respectively

		self.cycle_relative_times = timings.pixel_time_offsets
		self.cycle_time = timings.cycle_time

		pixel_time_offsets=pixel_time_offsets,
		dimensions=dimensions,

		'relative to the starting time/timestamp of a '
		'ROISeriesWithPixelTimeOffsets.',

		self.raw_pixel_time_offsets = timings.pixel_time_offsets
		self.cycle_time = timings.cycle_time

		else:
		raise ValueError('Unsupported LabView version for timings {}.'.format(self.labview_version))



		def test_pixel_time_offsets_for_roi_v231(synthetic_timings_v231):
		roi_0_offsets = synthetic_timings_v231.pixel_time_offsets[0]



		def test_pixel_time_offsets_for_roi_pre2018(synthetic_timings_pre2018):
		roi_0_offsets = synthetic_timings_pre2018.pixel_time_offsets[0]

New pixel time offsets and cycle times #58

New pixel time offsets and cycle times #58

Conversation

alessandrofelder commented Jul 2, 2020

ageorgou 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

ageorgou 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

ageorgou 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

Choose a reason for hiding this comment

ageorgou commented Jul 24, 2020 • edited by alessandrofelder Loading

ageorgou commented Jul 24, 2020 •

edited by alessandrofelder

Loading