HTML repr: Remove double nesting in processing modules

[h-mayorquin]

This PR modifies the html repr of the NWBFiles so the processing modules do not display a double nesting with the data_interfaces attribute:

Left is the new behavior right is the old behavior

This uses duck typing in the MultiContainerinterface to eliminate the double nesting (right on the image above) for the ProcessingModule. As ProcessingModule lives on pynwb I can't do the more natural solution of either overriding the method there (which would imply having some of the html repr code in pynwb) or checking of class instance here (which risks a circular dependency).

Checklist

• Did you update CHANGELOG.md with your changes?
• Does the PR clearly describe the problem and the solution?
• Have you reviewed our Contributing Guide?
• Does the PR use "Fix #XXX" notation to tell GitHub to close the relevant issue numbered XXX when the PR is merged?

[codecov]

Codecov Report

❌ Patch coverage is 75.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.44%. Comparing base (00419e8) to head (6448143).
⚠️ Report is 1 commits behind head on dev.

Files with missing lines	Patch %	Lines
src/hdmf/container.py	75.00%	1 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1354      +/-   ##
==========================================
- Coverage   92.46%   92.44%   -0.03%     
==========================================
  Files          42       42              
  Lines        9730     9742      +12     
  Branches     1975     1978       +3     
==========================================
+ Hits         8997     9006       +9     
- Misses        461      462       +1     
- Partials      272      274       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[oruebel]

Looks good to me

[rly]

As ProcessingModule lives on pynwb I can't do the more natural solution of either overriding the method there (which would imply having some of the html repr code in pynwb)

I understand that it's nice to have all the html code in one place, but I don't think it is such a bad thing to have HTML repr code in PyNWB. Each subclass should be able to control how it's displayed in HTML. We already have overrides for TimeSeries in PyNWB: https://github.com/NeurodataWithoutBorders/pynwb/blob/7e100c2d06ba73cd9ad873a7b1ee0a2d5156ce8a/src/pynwb/base.py#L352

In the approach of this PR, it might be confusing to maintainers who are wondering why there is a special case for PyNWB in HDMF, and this function could grow in complexity as more special cases get added. If someone builds an extension that happens to have a MultiContainerInterface with attribute name "data_interfaces", they may be confused as to why this behavior is different. These are rare cases though.

I would prefer to have this in PyNWB.

[h-mayorquin]

I would prefer to have this in PyNWB.

I am OK with it.

The only downside is the slower cycle of release : (

[h-mayorquin]

Moved to pynwb here:
NeurodataWithoutBorders/pynwb#2149

Feel free to close this then.

[rly]

Now that I have tested this and the other PR locally, it would be nice to make this work for all MultiContainerInterface classes that have only one grouping attribute. These classes allow the user to get the child by name without knowing the attribute name, e.g., nwbfile.processing["behavior"]["Position"]["SpatialSeries"] instead of nwbfile.processing["behavior"].data_interfaces["Position"].spatial_series["SpatialSeries"]. Could you update this so that if there is only one grouping attribute, the grouping attribute is removed from the html repr (e.g., "data_interfaces", "spatial_series")? The pynwb PR would not be necessary anymore, I think.

See

hdmf/src/hdmf/container.py

Lines 1278 to 1282 in 35f123e

	# make __getitem__ (square bracket access) only if one conf type is defined
	if len(clsconf) == 1:
	attr = clsconf[0].get('attr')
	container_type = clsconf[0].get('type')
	setattr(cls, '__getitem__', cls.__make_getitem(attr, container_type))

[h-mayorquin]

Ok, I fixed this and the tooltip:

@rly

The tooltip looks like this here because I was testing on the dev branch but is fixed in this branch.

[rly]

Great! Thank you so much @h-mayorquin !