Inconsistency between APIs for NWBContainer, NWBData, and DynamicTable

[rly]

NWBContainer and DynamicTable represents groups and NWBData represents datasets. The _fieldsname property differs between the three of them and is confusing and unintuitive.

NWBContainer._fieldsname is '__nwbfields__' DynamicTable._fieldsname is '__fields__' NWBData._fieldsname is '__fields__'

This is because only NWBContainer redefines the _fieldsname attribute. The others do not.

This is confusing for 1) designing custom APIs and 2) maintaining the codebase, and I do not see the need to have a special fields name unique to PyNWB.

This has most recently caused errors in #1260 but has also caused previous errors. For example, all tables in PyNWB use __fields__ and not __nwbfields__ unlike all non-table classes in core PyNWB, which caused #1108.

I suggest changing NWBContainer._fieldsname from '__nwbfields__' to '__fields__' but keeping support for '__nwbfields__', if present in the class, for backward compatibility.

Checklist

• [x] Have you ensured the feature or change was not already reported?
• [x] Have you included a brief and descriptive title?
• [x] Have you included a clear description of the problem you are trying to solve?
• [x] Have you included a minimal code snippet that reproduces the issue you are encountering?
• [x] Have you checked our Contributing document?

[rly]

Alternatively, we could redefine DynamicTable within PyNWB and change the _fieldsname attribute to __nwbfields__ and also set NWBData._fieldsname to __nwbfields__. This may be easier to implement, BUT then code that imports hdmf.common.DynamicTable would no longer be equivalent to code that imports pynwb.core.DynamicTable, which will probably break some other people's code and be generally confusing.

[rly]

Related to this, currently all NWBData subtypes define __nwbfields__ but this is a big - the fields name needs to be __fields__ for the getters and setters to be generated correctly. For example, the generated setter blocks resetting of attributes. So, the following code is possible when it should not be because the custom setter was not created:

import pynwb
aa = pynwb.base.Image(name='aa', data=[[]], description='aaa')
aa.description = 'bbb'

[rly]

This bug hit me and @mavaylon today. Any class that subclasses DynamicTable has to use __fields__ instead of __nwbfields__. We could get around that in a similar way to when we extracted MultiContainerInterface which is to redefine DynamicTable in pynwb where it inherits from NWBDataInterface or NWBContainer (which changes __fields__ to __nwbfields__): https://github.com/NeurodataWithoutBorders/pynwb/blob/497c21563e75a4a004d5c6a49186cf94c6e75441/src/pynwb/core.py#L132-L135

OR just change all of __nwbfields__ in PyNWB to be __fields__ to prevent future headaches for other classes imported from HDMF.

[oruebel]

@ajtritt what are your thoughts on this? I’d be Ok with changing things to fields in PyNWB if that doesn’t break extensions.

@rly I assume this would then also change NWBContainer to no longer define __nwbfields__ or would NWBContainer simply merge __nwbfields__ into __fields__ to make sure extensions using __nwbfields__ can keep using it?

[ajtritt]

Yeah, I would say change it so they're all fields