Allow reassigning and resetting AbstractContainer attributes/fields

[rly]

Motivation

Fix #411, fix #865

Major change to allow setting attributes on Container objects to new values, including None. If the attributes were read from a file (i.e. container_source is not None), then a warning is raised.

Note that we currently do a lot of validation at the constructor level (type validation, shape validation) in docval. We do not do that for each setter. This can result in errors on write if the user is not careful. We could instead move validation of attributes to outside of docval and into the setter. That is significantly more development, but I think worthwhile in the long run. I will open a separate issue for this.

Checklist

• Did you update CHANGELOG.md with your changes?
• Have you checked our Contributing document?
• Have you ensured the PR clearly describes the problem and the solution?
• Is your contribution compliant with our coding style? This can be checked running ruff from the source directory.
• Have you checked to ensure that there aren't other open Pull Requests for the same change?
• Have you included the relevant issue number using "Fix #XXX" notation where XXX is the issue number? By including "Fix #XXX" you allow GitHub to close issue #XXX when the PR is merged.

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.06% 🎉

Comparison is base (64a444f) 88.33% compared to head (a2c02ee) 88.39%.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev     #868      +/-   ##
==========================================
+ Coverage   88.33%   88.39%   +0.06%     
==========================================
  Files          45       45              
  Lines        9283     9288       +5     
  Branches     2651     2652       +1     
==========================================
+ Hits         8200     8210      +10     
+ Misses        765      762       -3     
+ Partials      318      316       -2     

Files Changed	Coverage Δ
src/hdmf/common/table.py	85.88% <100.00%> (+0.35%)	⬆️
src/hdmf/container.py	91.61% <100.00%> (+0.06%)	⬆️

... and 1 file with indirect coverage changes

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

[rly]

PyNWB tests need to be updated because container attributes that were previously set to None resulted in no entry in self.fields. This has the primary side effect that the repr of an object now shows all the attributes that are set to None. This affects the repr of all objects with optional fields. For example:

  Fields:
    comments: no comments
-   continuity: None
-   control: None
-   control_description: None
    conversion: 1.0
    data: [1000 2000 3000]
    description: no description
    interval: 1
    offset: 0.0
-   rate: None
    resolution: -1.0
-   starting_time: None
    timestamps: [1. 2. 3.]
    timestamps_unit: seconds
    unit: unit

The attributes marked with a dash are new additions to the repr. I think it is good to be more transparent and complete. If you call timeseries.rate, when rate was not set, None will be returned. In the case of TimeSeries where (timestamps) or (starting_time and rate) should be set, this might be confusing to users.

I made this change to allow users to reset an attribute to None and make the behavior not dependent on the current state of the container. What do you think? Is this a worthwhile change?

If not, I can add logic in the setters that says if the value is None AND the field is not set, do nothing and return.

[bendichter]

Thanks for tackling this, @rly ! I'm testing incorporation with catalystneuro/neuroconv#475 now..

[bendichter]

While I like the ability to change values, I think too much freedom could be confusing for users. For instance, now a user is able to run:

from pynwb.testing.mock.base import mock_TimeSeries

ts = mock_TimeSeries(timestamps=[1,2,3,4], rate=None)
ts.rate = 5

This will create a TimeSeries object that has both timestamps and rate, which is illegal, but this won't be flagged until the user goes to write.

Would it be possible to perform validation every time a Container class is modified? Or maybe we can make it so only values that have been set as non-None can be modified. Or maybe there might be some other way to do this?

[rly]

@bendichter Good point. Yes, we can do that - we can call a generic validation function in the generic setter. Moving all validation in PyNWB classes to that function and making it work for auto-generated classes will take some more work though, but I think ultimately this would be a large improvement in usability.

For now, what if we constrain the changes in this PR to allow reassigning only Data.data? Type and shape validation will not occur (but it doesn't occur anyway if you pass in a DataIO object).

[bendichter]

For now, what if we constrain the changes in this PR to allow reassigning only Data.data? Type and shape validation will not occur (but it doesn't occur anyway if you pass in a DataIO object).

Sounds good to me!

[bendichter]

wait, no, I need to overwrite TimeSeries.data and TimeSeries.timestamps as well. In fact, I'm not able to do that with the current branch:

from pynwb.testing.mock.base import mock_TimeSeries

ts = mock_TimeSeries(timestamps=[1,2,3,4], rate=None)
ts.rate = 5  # this works
ts.data = [2,3,4,5]  # this does not work

---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
Cell In [6], line 7
      5 ts = mock_TimeSeries(timestamps=[1,2,3,4], rate=None)
      6 ts.rate = 5
----> 7 ts.data = [2,3,4,5]

AttributeError: can't set attribute

[rly]

Note: Now that the IO object is available on Container objects, for attributes, we may be able to easily modify any written attributes. We would need to store the path to the attribute and have a setter that detects if it was read from a file, the file is writeable, and then go to the object, and overwrite the value of the attribute.