NeurodataWithoutBorders / matnwb

A Matlab interface for reading and writing NWB files
BSD 2-Clause "Simplified" License
50 stars 32 forks source link

TimeSeries.timestamps.interval stored as double #95

Closed bendichter closed 5 years ago

bendichter commented 5 years ago

TimeSeries.timestamps.interval should be an int32: https://github.com/NeurodataWithoutBorders/matnwb/blob/fc9064e913b940a40fe2f5e6635045dc0b136054/schema/core/nwb.base.yaml#L206-L214

however it is stored as a double, demonstrated by the following code:

date = datetime(2018, 3, 1, 12, 0, 0);
session_start_time = datetime(date,'Format','yyyy-MM-dd''T''HH:mm:SSZZ',...
    'TimeZone','local');
file = nwbfile( 'source', 'acquired on rig2', ...
    'session_description', 'a test NWB File', ...
    'identifier', 'mouse004_day4', ...
    'session_start_time', session_start_time);

ts = types.core.TimeSeries(...
        'data', (100:10:190) .', ...
        'data_unit', 'SIunit', ...
        'timestamps', (0:9) .', ...
        'data_resolution', 0.1);
file.acquisition.set('test_timeseries', ts);

class(file.acquisition.get('test_timeseries').timestamps_interval)

nwbExport(file, 'test_timeseries.nwb')

'double'

The value is written to the HDF5 file as a double as well.

bendichter commented 5 years ago

I'm not 100% sure what interval means here. The schema docs are not much help.

On Sun, Jan 20, 2019, 7:06 PM Tom Davidson <notifications@github.com wrote:

Is ‘Interval’ meant to be the elapsed time between samples (I.e. 1/rate)? If so, then i think it should be a float.

Sent from my phone

On Jan 20, 2019, at 8:41 AM, Ben Dichter notifications@github.com wrote:

TimeSeries.timestamps.interval should be an int32:

https://github.com/NeurodataWithoutBorders/matnwb/blob/fc9064e913b940a40fe2f5e6635045dc0b136054/schema/core/nwb.base.yaml#L206-L214

however it is stored as a double, demonstrated by the following code:

date = datetime(2018, 3, 1, 12, 0, 0); session_start_time = datetime(date,'Format','yyyy-MM-dd''T''HH:mm:SSZZ',... 'TimeZone','local'); file = nwbfile( 'source', 'acquired on rig2', ... 'session_description', 'a test NWB File', ... 'identifier', 'mouse004_day4', ... 'session_start_time', session_start_time);

ts = types.core.TimeSeries(... 'data', (100:10:190) .', ... 'data_unit', 'SIunit', ... 'timestamps', (0:9) .', ... 'data_resolution', 0.1); file.acquisition.set('test_timeseries', ts);

class(file.acquisition.get('test_timeseries').timestamps_interval)

nwbExport(file, 'test_timeseries.nwb') 'double'

The value is written to the HDF5 file as a double as well.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/NeurodataWithoutBorders/matnwb/issues/95#issuecomment-455883612, or mute the thread https://github.com/notifications/unsubscribe-auth/AAziErgy5ZS8N8hDnbuHyUdZmQAu677Yks5vFKH9gaJpZM4aJuxf .

tjd2002 commented 5 years ago

Maybe it’s meant for the case where there is only a single timestamp written per buffer, in which case the interval would be the ‘stride’ or number of elements in the buffer. A sensible default would be 1 (a timestamp for each sample). In this case you are right that it should be an int.

bendichter commented 5 years ago

I'm not trying to make a case one way or the other on the type, I am just trying to fix an inconsistency between the schema and the io tools. I assume this was an oversight by matnwb, so that's what I'm fixing. I would be open to changing the type depending on how we choose to interpret it. Though I think your second explanation is probably right, so maybe int is best after all.

On Sun, Jan 20, 2019, 10:59 PM Tom Davidson <notifications@github.com wrote:

Maybe it’s meant for the case where there is only a single timestamp written per buffer, in which case the interval would be the ‘stride’ or number of elements in the buffer. A sensible default would be 1 (a timestamp for each sample). In this case you are right that it should be an int.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/NeurodataWithoutBorders/matnwb/issues/95#issuecomment-455901922, or mute the thread https://github.com/notifications/unsubscribe-auth/AAziEqMvuZlSSqb35EV6IPdD1h7qFI37ks5vFNjNgaJpZM4aJuxf .

lawrence-mbf commented 5 years ago

11464aee37a6ced83a3cc3a21e313662c877c0e5 Requires regeneration.

bendichter commented 5 years ago

thanks

bendichter commented 5 years ago

now write is broken for TimeSeries:

date = datetime(2018, 3, 1, 12, 0, 0);
session_start_time = datetime(date,'Format','yyyy-MM-dd''T''HH:mm:SSZZ',...
    'TimeZone','local');
file = nwbfile( 'source', 'acquired on rig2', ...
    'session_description', 'a test NWB File', ...
    'identifier', 'mouse004_day4', ...
    'session_start_time', session_start_time);

ts = types.core.TimeSeries(...
        'data', (100:10:190) .', ...
        'data_unit', 'SIunit', ...
        'timestamps', (0:9) .', ...
        'data_resolution', 0.1);
file.acquisition.set('test_timeseries', ts);

nwbExport(file, 'test_timeseries.nwb')
Error using hdf5lib2
The class of input data must be integer instead of int32 when the HDF5
class is H5T_STD_I64LE.

Error in H5A.write (line 53)
H5ML.hdf5lib2('H5Awrite', attr_id, type_id, buf);

Error in io.writeAttribute (line 24)
H5A.write(id, tid, data);

Error in types.core.TimeSeries/export (line 246)
            io.writeAttribute(fid, [fullpath '/timestamps/interval'],
            class(obj.timestamps_interval), obj.timestamps_interval,
            false);

Error in types.untyped.Set/export (line 147)
                    refs = v.export(fid, propfp, refs);

Error in types.core.NWBFile/export (line 508)
            refs = obj.acquisition.export(fid, [fullpath
            '/acquisition'], refs);

Error in nwbfile/export (line 39)
                refs = export@types.core.NWBFile(obj, fid, '/', {});

Error in nwbExport (line 34)
    export(nwb(i), fn);

Error in write_timeseries (line 16)
nwbExport(file, 'test_timeseries.nwb')
lawrence-mbf commented 5 years ago

Strange, that works for me. did you generateCore?

bendichter commented 5 years ago

I may have messed up with git. Let me reset and try again.

On Tue, Jan 22, 2019 at 5:57 PM ln-vidrio notifications@github.com wrote:

Strange, that works for me. did you generateCore?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/NeurodataWithoutBorders/matnwb/issues/95#issuecomment-456476229, or mute the thread https://github.com/notifications/unsubscribe-auth/AAziEsVrSWzktkg13WNWQMEJK9kxNAbjks5vF0L7gaJpZM4aJuxf .

--

Ben Dichter, PhD Data Science Consultant

bendichter commented 5 years ago

hmm I don't know...

>> !git fetch origin
>> !git reset --hard origin/master
HEAD is now at 11464ae fix for type correction.
>> generateCore('schema/core/nwb.namespace.yaml')

date = datetime(2018, 3, 1, 12, 0, 0);
session_start_time = datetime(date,'Format','yyyy-MM-dd''T''HH:mm:SSZZ',...
    'TimeZone','local');
file = nwbfile( 'source', 'acquired on rig2', ...
    'session_description', 'a test NWB File', ...
    'identifier', 'mouse004_day4', ...
    'session_start_time', session_start_time);

ts = types.core.TimeSeries(...
        'data', (100:10:190) .', ...
        'data_unit', 'SIunit', ...
        'timestamps', (0:9) .', ...
        'data_resolution', 0.1);
file.acquisition.set('test_timeseries', ts);

nwbExport(file, 'test_timeseries.nwb')

I get the same error

bendichter commented 5 years ago

I'm using a mac, if that info is helpful. Let me know if I can give you any more information that could help you track this down.

lawrence-mbf commented 5 years ago

So it looks like the HDF5 type mapping is unexpectedly using a larger type than what MATLAB expected. H5T_NATIVE_LONG seems to map to H5T_STD_I64LE. That's a little annoying, I'll look into it.

lawrence-mbf commented 5 years ago

try this. You don't need to regenerate. cce66be12c40382e9270535371b1eb788460c80f

bendichter commented 5 years ago

Yup, works. Thanks for the fix! :-)