NeurodataWithoutBorders / matnwb

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

TimeSeriesIOTest not passing #106

Closed bendichter closed 5 years ago

bendichter commented 5 years ago
================================================================================
Verification failed in tests.system.TimeSeriesIOTest/testOutToPyNWB.

    ----------------
    Test Diagnostic:
    ----------------

    ======================================================================
    FAIL: testInFromMatNWB (PyNWBIOTest.TimeSeriesIOTest) (container_type='TimeSeries', nwbfield='data')
    ----------------------------------------------------------------------
    Traceback (most recent call last):
      File "/Users/bendichter/dev/matnwb/+tests/+system/PyNWBIOTest.py", line 99, in assertContainerEqual
        self.assertEqual(f1, f2)
    AssertionError: <HDF5 dataset "data": shape (10,), type "<f8"> != [100, 110, 120, 130, 140, 150, 160, 170, 180, 190]

    ======================================================================
    FAIL: testInFromMatNWB (PyNWBIOTest.TimeSeriesIOTest) (container_type='TimeSeries', nwbfield='timestamps')
    ----------------------------------------------------------------------
    Traceback (most recent call last):
      File "/Users/bendichter/dev/matnwb/+tests/+system/PyNWBIOTest.py", line 99, in assertContainerEqual
        self.assertEqual(f1, f2)
    AssertionError: <HDF5 dataset "timestamps": shape (10,), type "<f8"> != array([0., 1., 2., 3., 4., 5., 6., 7., 8., 9.])

    ----------------------------------------------------------------------
    Ran 1 test in 0.038s

    FAILED (failures=2)

    ------------------
    Stack Information:
    ------------------
    In /Users/bendichter/dev/matnwb/+tests/+system/PyNWBIOTest.m (PyNWBIOTest.testOutToPyNWB) at 15
================================================================================
.
================================================================================
Verification failed in tests.system.TimeSeriesIOTest/testInFromPyNWB.

    ----------------
    Test Diagnostic:
    ----------------
    Values for property 'timestamps_interval' are not equal

    ---------------------
    Framework Diagnostic:
    ---------------------
    verifyEqual failed.
    --> Classes do not match.

        Actual Class:
            int64
        Expected Class:
            int32

    Actual Value:
      int64

       1
    Expected Value:
      int32

       1

    ------------------
    Stack Information:
    ------------------
    In /Users/bendichter/dev/matnwb/+tests/+system/RoundTripTest.m (RoundTripTest.verifyContainerEqual) at 75
    In /Users/bendichter/dev/matnwb/+tests/+system/PyNWBIOTest.m (PyNWBIOTest.testInFromPyNWB) at 28
================================================================================

================================================================================
Verification failed in tests.system.TimeSeriesIOTest/testInFromPyNWB.

    ----------------
    Test Diagnostic:
    ----------------
    Values for property 'data' are not equal

    ---------------------
    Framework Diagnostic:
    ---------------------
    verifyEqual failed.
    --> Classes do not match.

        Actual Class:
            int64
        Expected Class:
            double

    Actual Value:
      10×1 int64 column vector

       100
       110
       120
       130
       140
       150
       160
       170
       180
       190
    Expected Value:
       100
       110
       120
       130
       140
       150
       160
       170
       180
       190

    ------------------
    Stack Information:
    ------------------
    In /Users/bendichter/dev/matnwb/+tests/+system/RoundTripTest.m (RoundTripTest.verifyContainerEqual) at 75
    In /Users/bendichter/dev/matnwb/+tests/+system/PyNWBIOTest.m (PyNWBIOTest.testInFromPyNWB) at 28
================================================================================
lawrence-mbf commented 5 years ago

So I believe #107 fixes tests.system.TimeSeriesIOTest/testInFromPyNWB as I am unable to reproduce that now.

lawrence-mbf commented 5 years ago

It looks like pynwb returns HDF5 Data objects instead of arrays now. Adding a check in assertContainerEqual to see if it's a h5py.Dataset and using slice syntax seems to work, though I'm unsure if that's the best idea.

import h5py
.
.
.
if isinstance(f1, (float, np.float32, np.float16)):
    npt.assert_almost_equal(f1, f2)
elif isinstance(f1, h5py.Dataset):
    self.assertEqual(f1[:], f2)
else:
    self.assertEqual(f1, f2)
bendichter commented 5 years ago

Yeah I think that would be fine. Alternatively, would it make sense to use pynwb's assertContainerEqual?

https://github.com/NeurodataWithoutBorders/pynwb/blob/d68bf3a24a5cc7c709c4bf13300da1e334740d80/tests/integration/ui_write/base.py#L79-L133

lawrence-mbf commented 5 years ago

That looks like it runs into the same issue. Does that one work on roundtrips?

bendichter commented 5 years ago

hmm, yeah I think so, but I'm not sure where reading h5py datasets is handled. Maybe before that function.

lawrence-mbf commented 5 years ago

So debugging the pynwb integration tests revealed a few interesting things: 1) The documentation for assertContainerEqual under base.py is incorrect as TestMapRoundTrip.test_roundtrip is clearly not comparing it in that order. This is important because... 2) Neither the type nor the actual contents in any read array is ever checked. In our case, this is the branch that is executed for the nwbfield "data" for TimeSeries.

Making this behavior consistent in MatNWB is easy but I don't know if that's intended or not.