add age__reference and adjust tests

[bendichter]

companion to NeurodataWithoutBorders/nwb-schema#525

[codecov]

Codecov Report

Merging #1540 (e4d1186) into dev (f4bd3bc) will increase coverage by 0.48%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev    #1540      +/-   ##
==========================================
+ Coverage   91.25%   91.74%   +0.48%     
==========================================
  Files          25       26       +1     
  Lines        2515     2543      +28     
  Branches      477      485       +8     
==========================================
+ Hits         2295     2333      +38     
+ Misses        139      134       -5     
+ Partials       81       76       -5     

Flag	Coverage Δ
integration	71.09% <93.54%> (+0.87%)	⬆️
unit	83.87% <35.48%> (-0.62%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/pynwb/file.py	88.08% <100.00%> (+0.08%)	⬆️
src/pynwb/io/file.py	97.07% <100.00%> (+2.73%)	⬆️
src/pynwb/io/utils.py	100.00% <100.00%> (ø)
src/pynwb/io/base.py	91.35% <0.00%> (+7.40%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[rly]

@bendichter Can you add a roundtrip test and a backwards compatibility check that checks that None is returned if age__reference is not in the file?

[bendichter]

@rly In writing this test, I came up against an error in hdmf:

class TestSubjectAgeReference(TestSubjectIO):

    def setUpContainer(self):
        """ Return the test Subject """
        return Subject(
            age='P90D',
            age__reference=None,
            description='A rat',
            genotype='WT',
            sex='M',
            species='Rattus norvegicus',
            subject_id='RAT123',
            weight='2 kg',
            date_of_birth=datetime(1970, 1, 1, 12, tzinfo=tzutc()),
            strain='my_strain',
        )

args = (<[AttributeError("'Subject' object has no attribute '_AbstractContainer__name'") raised in repr()] Subject object at 0x7fa562325130>,)
kwargs = {'age': 'P90D', 'age__reference': None, 'date_of_birth': datetime.datetime(1970, 1, 1, 12, 0, tzinfo=tzutc()), 'description': 'A rat', ...}

    def _check_args(args, kwargs):
        """Parse and check arguments to decorated function. Raise warnings and errors as appropriate."""
        # this function was separated from func_call() in order to make stepping through lines of code using pdb
        # easier
        parsed = __parse_args(
            loc_val,
            args[1:] if is_method else args,
            kwargs,
            enforce_type=enforce_type,
            enforce_shape=enforce_shape,
            allow_extra=allow_extra,
            allow_positional=allow_positional
        )
    
        parse_warnings = parsed.get('future_warnings')
        if parse_warnings:
            msg = '%s: %s' % (func.__qualname__, ', '.join(parse_warnings))
            warnings.warn(msg, FutureWarning)
    
        for error_type, ExceptionType in (('type_errors', TypeError),
                                          ('value_errors', ValueError),
                                          ('syntax_errors', SyntaxError)):
            parse_err = parsed.get(error_type)
            if parse_err:
                msg = '%s: %s' % (func.__qualname__, ', '.join(parse_err))
>               raise ExceptionType(msg)
E               TypeError: Subject.__init__: incorrect type for 'age__reference' (got 'NoneType', expected 'str or NoneType')

It looks like docval does not accept an input of None

[bendichter]

It looks like there is an allow_none flag, but I don't see how to set it: https://github.com/hdmf-dev/hdmf/blob/bb156780017a5ca19b7f148f4452862761bef751/src/hdmf/utils.py#L70

[rly]

@bendichter Can you add

1. a roundtrip test and
2. a backwards compatibility check that checks that age__reference = None if age__reference is not found in the file?

[rly]

tl;dr I think we should not allow the case where Subject(age__reference=None) sets the age_reference to None which means no value is written, and then on read, the value is set to the default value of "birth". Instead, you can either pass a valid value for age__reference or don't include it at all. I updated the PR accordingly.

Explanation:
Based on the spec for default_value, if the schema says default_value: birth and a file with that schema is read with the attribute missing, then PyNWB should read the value as "birth". However, for old files generated when this attribute did not exist, I think PyNWB should read the value as None because it is governed by an older schema. This is mostly the behavior created by this PR.

Use cases:

1. Read NWB 2.6 file, read age__reference = "birth" (or gestational) - value is read as "birth" (or gestational) ✅
2. Read NWB 2.6 file, age__reference is missing - value is read as "birth" ✅
3. Read NWB 2.0-2.5 file, age__reference is missing - value is read as None ✅
4. Write NWB 2.6 file, Subject(age__reference="birth") (or gestational) - value is written as "birth" (or gestational) ✅
5. Write NWB 2.6 file, Subject(age__reference=None) - no value is written ✅
   a) In memory before write, age__reference has value None but I think either:
   A) age_reference should be set to the default value "birth". Otherwise, roundtrip tests fail, because on read, age__reference is read as "birth". Or
   B) we should just not allow this case. If the user wants to set age__reference, then only "birth" and "gestational" are allowed. It would be confusing if the user explicitly sets the value to None and it gets changed to "birth" in A.
   So I think we should do B and I have updated the PR accordingly.❓
6. Write NWB 2.6 file, Subject() - no value is written ✅
   a) In memory before write, age__reference has value "birth" and on read, age__reference is read as "birth" ✅

To implement what I think the logic should be above, then on read of an NWB file, the value of age__reference when it is not in the file depends on the schema version. See the mapper code I added.

[rly]

I will add a unit test for get_nwb_version later today and then this should be good to go.

[rly]

@bendichter This is good to go now. Please review my changes.

Note that when a user calls Subject(age__reference=None) or Subject(), then age__reference will be set to the default value "birth" for reasons described above. The only way that age__reference can be None is if the file uses NWB schema < 2.6.0 and the attribute is missing.