Issues with writing the new extension on an NWB file

[Hamidreza-Alimohammadi]

Hi all,
As mentioned in the slack channel, I've been working with developing a new extension to incorporate ECG data standards to the NWB. Here's a link to the repository:
https://github.com/Defense-Circuits-Lab/ndx_ecg
The specs have been successfully defined and the defined groups can be also successfully imported and used to store what they are intended to store (As is testable by running a sample script in src/pynwb/tests/test_script_ecg.py), however, when it comes to writing it on the file this error comes up:
hdmf.build.errors.OrphanContainerBuildError: ecg_raw (ecg_raw): Linked ECGChannelsGroup '' has no parent. Remove the link or ensure the linked container is added properly.
ECGChannelsGroup is an extension to the NWBContainer base and it seems to be orphaned. I tried to take care of this problem by removing this NWBContainer extension and implementing it's data through a direct dataset into the CardiacSeries itself and put a soft link to the extended Device-type ECGRecDevice, but as mentioned in the slack got another error:
TypeError: GroupBuilder.set_dataset: incorrect type for 'builder' (got 'GroupBuilder', expected 'DatasetBuilder')
So, I would definitely appreciate any comments, and would really prefer if the already exisiting extension scheme at the provided repository could be put into working.
I hope I have provided enough information regarding this issue,
Thank you for your time in advance!

[rly]

Hi @Hamidreza-Alimohammadi , I ran your example script after adding

with NWBHDF5IO('test_ecg.nwb', 'w') as io:
    io.write(nwb_file)

at the end.

I got the error

hdmf.build.errors.OrphanContainerBuildError: ecg_raw (ecg_raw): Linked ECGChannelsGroup '' has no parent. Remove the link or ensure the linked container is added properly.

as you reported.

Looking at your script and extension, I found a few issues and likely errors. It looks like you create a generic DynamicTable for your ECG electrodes and a generic DynamicTable for your recording channels. You add a column to recording_channels_table called electrodes. When you add rows to this table, you say:

recording_channels_table.add_row(
    channel_name='ch_0',
    channel_type='single',
    electrodes=DynamicTableRegion(
        name='',
        data=[0],
        table=ecg_electrodes_table,
        description=''
    )
)

This is not the correct usage of DynamicTableRegion. A DynamicTableRegion should be a column of the parent table, where the values represent references to row indices of the table argument. I think to build this recording_channels_table the way that you want, you should define the electrodes column as a DynamicTableRegion. I see that some rows reference more than one column of the other table. So this is a ragged (or indexed) table. You can do that in two ways:

1. define the columns and then pass those columns to the DynamicTable.__init__ -- see https://hdmf.readthedocs.io/en/stable/tutorials/plot_dynamictable_howto.html#referencing-rows-of-other-tables for examples
2. instead of:

   recording_channels_table.add_column(
   	name='electrodes',
   	description='descriptive information the corresponding electrode(s)'
   )

   Use:

   recording_channels_table.add_column(
       name='electrodes',
       description='descriptive information the corresponding electrode(s)',
       table=ecg_electrodes_table,
       index=True,
   )

Option 2 is probably easier.

Then to add rows, you would use something like:

recording_channels_table.add_row(
    channel_name='ch_0',
    channel_type='single',
    electrodes=[0]
)
recording_channels_table.add_row(
    channel_name='ch_1',
    channel_type='differential',
    electrodes=[0, 1]
)

The OrphanContainerBuildError error that you see above occurs because your ECGChannelsGroup defined in https://github.com/Defense-Circuits-Lab/ndx_ecg/blob/main/src/pynwb/tests/test_script_ecg.py#L127 is never added to the NWB file. You link to the group in your CardiacSeries object, but a link is a pointer, like a shortcut or alias on your filesystem. The CardiacSeries object does not contain the ECGChannelsGroup within it. So the ECGChannelsGroup needs to be added elsewhere to the file.

You defined ECGChannelsGroup as a subtype of NWBContainer but that can make it hard to put into the file because most places in NWBFile that accept data types accept subtypes of NWBDataInterface or DynamicTable. There are two easy solutions to this.

1. define a new type that extends LabMetaData and contains an instance of ECGChannelsGroup (see https://nwb-overview.readthedocs.io/en/latest/extensions_tutorial/extension_examples/labmetadata_extension.html for an example). Then you can add your ECGChannelsGroup to your ECGMetaData object, and add that ECGMetaData to your NWB file using nwb_file.add_lab_meta_data(ecg_meta_data).
2. have ECGChannelsGroup extend NWBDataInterface instead of NWBContainer. Then you can add your ECGChannelsGroup using something like nwbfile.add_acquisition. NWBDataInterface is really meant for data and not metadata like this channels grouping (it might be odd to find channel metadata in the acquisition group), so I would recommend option 1.

Once you solve that, the script now generates this error:

TypeError: GroupBuilder.set_dataset: incorrect type for 'builder' (got 'GroupBuilder', expected 'DatasetBuilder')

which you reported.

This usually means a group (usually a subtype of NWBContainer was passed in for an argument that was specified as a dataset in the spec. Looking through your spec, I see that ECGChannelsGroup contains a dataset called data that is a DynamicTable. However, DynamicTable is a group, not a dataset. Here are two options to resolve this:

1. in your create_extension_spec.py file, change these lines https://github.com/Defense-Circuits-Lab/ndx_ecg/blob/main/src/spec/create_extension_spec.py#L179-L180 to say:

   groups:
       NWBGroupSpec(

   I think everything else can stay the same.
2. It looks like this ECGChannelsGroup simply holds a generic DynamicTable with two descriptions and a link to a ECGRecDevice. An alternate approach to creating a new group like this is to extend DynamicTable and add attributes and links to the extended DynamicTable. For example, something like:

   ecg_recording_channels = NWBGroupSpec(
       neurodata_type_def='ECGChannelsTable',
       neurodata_type_inc='DynamicTable',
       ...,
       attributes=...,
       links=...,
   )

   This ECGChannelsTable automatically inherits all of the properties of DynamicTable. You can even define your custom columns "channel_name", "channel_type", and "electrodes" at the spec level instead of in the python code to add the data.

Let me know how this goes and if you have any questions.

[Hamidreza-Alimohammadi]

Dear @rly,
Thank you very much for the comprehensive answer, I really do appreciate it.
I tried to incorporate your points in several versions, up to this last version (https://github.com/Hamidreza-Alimohammadi/ndx_ecg_fix/tree/main), in which I have eliminated the NWBContainer of ECGChannelsGroup and demanded the user to pass two additional arguments of electrodes and channels -both of which being DynamicTable instances- through the extended Device, i.e. ECGRecDevice.
Now, it's working fine, both the importing and writing goes smoothly, however, when I attempt to read the file (with this script to be exact):
file_ = NWBHDF5IO('nwbfile_ext.nwb', mode='r', load_namespaces=True).read()
it goes nuts throwing this error:
TypeError: CustomClassGenerator.set_init.<locals>.__init__: missing argument 'electrodes', missing argument 'channels'.
No idea what it is, I mean I can't see they are orphaned as the previous case with the NWBContainer; They are successfully passed through the ECGRecDevice as instances of DynamicTable subgroups and then successfully added to NWBFile.Devices, so I wouldn't assume if they are hanging somewhere out of the NWBfile or are they?!
I would appreciate your comments.
Thanks and Best

*Also as a side note, I've been thinking that it would be great to have an specific storing point for generic NWBContainers in the NWBFile, maybe something like NWBFile.add_custom_container('name'). Of course, I am no expert in software engineering and have no idea how hard or easy would it be to implement such thing, but one thing I know for sure: it really gives greater levels of freedom for developing extensions. As in my understanding, NWBContainers can be very general and if I can have write them somewhere on the NWBFile then linking them and using them would be awesome for a freely customized extension!

[Hamidreza-Alimohammadi]

Sorry... forgot to mention that the new test-run script is located here: src/pynwb/tests/test_ecg_script.py

[oruebel]

As a first check, can you check that the electrodes and channels are indeed written to the file. From the error it seems the class is being generated correctly but that the electrodes and channels may be missing, so a first thing is to confirm that they actually exists. To check you could either open the file with h5py, use the h5ls or h5dump command-line tool, or use a generic HDF5 viewer https://nwb-overview.readthedocs.io/en/latest/tools/hdftools/hdftools.html#hdf-tools

[Hamidreza-Alimohammadi]

Thanks for the reply,
Please, let me check something with you to see if I have got it right:
I have defined an extension to the Device which is ECGRecDevice, the definition goes like:

ecg_recording_device = NWBGroupSpec(
        neurodata_type_def='ECGRecDevice',
        neurodata_type_inc='Device',
        doc='',
        attributes=[
        ...
        ],
        links=[
            ...
        ],
        groups=[
            NWBGroupSpec(
                name='electrodes',
                neurodata_type_inc='DynamicTable',
                doc='A dynamic table of the implemented electrodes.',
            ),
            NWBGroupSpec(
                name='channels',
                neurodata_type_inc='DynamicTable',
                doc='A dynamic table of the ECG recording channels.',
            )
        ]
    )

I was wondering if by creating two instances of DynamicTable, and passing them through the ECGRecDevice, then adding the ECGRecDevice to NWBFile.Devices, should I expect them to be properly saved and written on the NWBFile? In other words, do electrodes and channels -as I have defined- get written on the file as if I had them passed as a datasets spec?!

[oruebel]

I think this is correct. @rly can you also take a look to make sure I'm not overlooking something.