oruebel / ndx-icephys-meta

NWB extensions for origanizing intracellular electrophysiology metadata
Other
6 stars 0 forks source link

Add/timereftype #64

Closed oruebel closed 3 years ago

oruebel commented 4 years ago

Fix #60 Add TimeSeriesReferenceVectorData type

rly commented 3 years ago

https://github.com/hdmf-dev/hdmf/pull/325 needs to be merged to HDMF first

oruebel commented 3 years ago

hdmf-dev/hdmf#325 has been closed because the feature has already been added to HDMF dev, but instead of colcls, the argument is named col_cls. This PR needs to be updated accordingly.

rly commented 3 years ago

This PR works now. Running the tests results in four errors which are also present in the master branch (see https://github.com/oruebel/ndx-icephys-meta/issues/77).

I am not sure if my fixes to test_icephys.ICEphysFileTests.test_add_icephys_meta_full_roundtrip are appropriate. Please verify @oruebel.

oruebel commented 3 years ago

@rly aside from the comment regarding the test, this looks good to me.

rly commented 3 years ago

@oruebel Thanks. I fixed those tests and to test the raggedness, I changed the first test so that one simultaneous recording includes more than one intracellular recording. I get the error:

Traceback (most recent call last):
  File "C:\Users\Ryan\Documents\NWB\ndx-icephys-meta\src\pynwb\ndx_icephys_meta\test\test_icephys.py", line 1247, in test_add_icephys_meta_full_roundtrip
    res = nwbfile.icephys_simultaneous_recordings[0]
  File "c:\users\ryan\documents\nwb\hdmf\src\hdmf\common\table.py", line 723, in __getitem__
    ret = self.get(key)
  File "c:\users\ryan\documents\nwb\hdmf\src\hdmf\common\table.py", line 829, in get
    ret = pd.DataFrame(retdf, index=pd.Index(name=self.id.name, data=id_index))
  File "C:\Users\Ryan\miniconda3\envs\dev\lib\site-packages\pandas\core\frame.py", line 411, in __init__
    mgr = init_dict(data, index, columns, dtype=dtype)
  File "C:\Users\Ryan\miniconda3\envs\dev\lib\site-packages\pandas\core\internals\construction.py", line 257, in init_dict
    return arrays_to_mgr(arrays, data_names, index, columns, dtype=dtype)
  File "C:\Users\Ryan\miniconda3\envs\dev\lib\site-packages\pandas\core\internals\construction.py", line 87, in arrays_to_mgr
    return create_block_manager_from_arrays(arrays, arr_names, axes)
  File "C:\Users\Ryan\miniconda3\envs\dev\lib\site-packages\pandas\core\internals\managers.py", line 1699, in create_block_manager_from_arrays
    construction_error(len(arrays), arrays[0].shape, axes, e)
  File "C:\Users\Ryan\miniconda3\envs\dev\lib\site-packages\pandas\core\internals\managers.py", line 1719, in construction_error
    "Shape of passed values is {0}, indices imply {1}".format(passed, implied)
ValueError: Shape of passed values is (2, 1), indices imply (1, 1)

I thought that these tables should create multi-indexes by default. Does it not?

The functionality of DynamicTable.get had been modified several times over the past year, so perhaps this line used to work and some behavior of DynamicTable.get needs to be reverted or changed.

oruebel commented 3 years ago

I thought that these tables should create multi-indexes by default. Does it not?

Yes, normally this should automatically create the ragged column since this is prescribed that way in the schema.

so perhaps this line used to work and some behavior of DynamicTable.get needs to be reverted or changed.

I'm pretty sure this worked at some point, but it certainly possible that it needs to be updated to deal with updated behavior in HDMF.