Fix: Mark required attribute- and link-properties as required

[ehennestad]

Fix #654

Motivation

Attributes and links that were required according to the NWB schemas were not required in MatNWB. This PR fixes that.
It also adds a warning if a dependent required attribute is missing on export. For example, an attribute might be required but part of an untyped dataset or group which is not required. If such a dataset or group is added, but a required attribute of that dataset or group is not, a warning is shown on export.

How to test the behavior?

% Example 1 - Required attribute. Ok before, fails now

nwbFile1 = NwbFile( ...
    'session_description', 'test file for nwb export', ...
    'identifier', 'export_test', ...
    'session_start_time', datetime("now", 'TimeZone', 'local') );
processingModule = types.core.ProcessingModule();
nwbFile1.processing.set('ModuleWithoutDescription', processingModule);
nwbExport(nwbFile1, "example1.nwb")


% Example 2 - Required link. Fails before, but for the wrong reason

nwbFile2 = NwbFile( ...
    'session_description', 'test file for nwb export', ...
    'identifier', 'export_test', ...
    'session_start_time', datetime("now", 'TimeZone', 'local') );
electrode = types.core.IntracellularElectrode('description', 'test');
nwbFile2.general_intracellular_ephys.set('Electrode', electrode)
nwbExport(nwbFile2, "example2.nwb")

% Example 3 - Required dependent attribute. Ok before, issues warning now

nwbFile3 = NwbFile( ...
    'session_description', 'test file for nwb export', ...
    'identifier', 'export_test', ...
    'session_start_time', datetime("now", 'TimeZone', 'local') );
nwbFile3.general_source_script = '.../nwbExportTest.m';
nwbExport(nwbFile3, "example3.nwb")

Checklist

• Have you ensured the PR description clearly describes the problem and solutions?
• Have you checked to ensure that there aren't other open or previously closed Pull Requests for the same change?
• If this PR fixes an issue, is the first line of the PR description fix #XX where XX is the issue number?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.21%. Comparing base (c300611) to head (dabfefe).

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #655      +/-   ##
==========================================
+ Coverage   95.18%   95.21%   +0.03%     
==========================================
  Files         124      124              
  Lines        5044     5077      +33     
==========================================
+ Hits         4801     4834      +33     
  Misses        243      243              

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

[bendichter]

I'm a bit confused by this one:

% Example 2 - Required link. Fails before, but for the wrong reason

nwbFile2 = NwbFile( ...
    'session_description', 'test file for nwb export', ...
    'identifier', 'export_test', ...
    'session_start_time', datetime("now", 'TimeZone', 'local') );
electrode = types.core.IntracellularElectrode('description', 'test');
nwbFile2.general_intracellular_ephys.set('Electrode', electrode)
nwbExport(nwbFile2, "example2.nwb")

To be a link it must be added somewhere else in the NWB file, right?

[bendichter]

% Example 3 - Required dependent attribute. Ok before, issues warning now

nwbFile3 = NwbFile( ...
    'session_description', 'test file for nwb export', ...
    'identifier', 'export_test', ...
    'session_start_time', datetime("now", 'TimeZone', 'local') );
nwbFile3.general_source_script = '.../nwbExportTest.m';
nwbExport(nwbFile3, "example3.nwb")

what was the missing attribute?

[bendichter]

And I guess these changes didn't cause any problems for the tutorials?

[ehennestad]

I'm a bit confused by this one:

% Example 2 - Required link. Fails before, but for the wrong reason

nwbFile2 = NwbFile( ...
    'session_description', 'test file for nwb export', ...
    'identifier', 'export_test', ...
    'session_start_time', datetime("now", 'TimeZone', 'local') );
electrode = types.core.IntracellularElectrode('description', 'test');
nwbFile2.general_intracellular_ephys.set('Electrode', electrode)
nwbExport(nwbFile2, "example2.nwb")

To be a link it must be added somewhere else in the NWB file, right?

Yes, this is not very clear.
types.core.IntracellularElectrode has the property device, which is a Link. Before, this property was not required, and now it is. Running the above code should fail, because the linked device is not added.
If you test the example on the master branch, it will fail, but the error message is unspecific, and should be considered a bug. If you run it on the PR branch, the error message will state the the required property device is missing

[ehennestad]

nwbFile3 = NwbFile( ...
    'session_description', 'test file for nwb export', ...
    'identifier', 'export_test', ...
    'session_start_time', datetime("now", 'TimeZone', 'local') );
nwbFile3.general_source_script = '.../nwbExportTest.m';
nwbExport(nwbFile3, "example3.nwb")

general_source_script_file_name . Running example 3 should now show this warning:

Warning: The property "general_source_script_file_name" of type "NwbFile" in file location "root" is required when the property
"general_source_script" is set. Please add a value for "general_source_script_file_name" and re-export. 

[ehennestad]

And I guess these changes didn't cause any problems for the tutorials?

All tests are passing

[bendichter]

If device is required by IntracellularElectrode, then wouldn't it be better to flag that in the constructor of IntracellularElectrode? Is that possible? This will likely cause backward compatibility issues where users were able to create NWB files that were out of spec and want to read them now. We solved issues like this, where we became more strict about the form of data, by creating checks that are run when an object is being created in memory (which throw errors) and separate checks when a file is being read (which throw warnings). Do you think something like that might be possible here?

[bendichter]

I guess the advantage of your approach is that if we flag these things on write then we don't have to worry about breaking read for old out-of-spec files. Since this is a pretty serious bug, I'm fine with pushing it forward in this way, even though it's not the ideal solution. But ultimately how would you feel about putting these checks in the class constructors?

[ehennestad]

@bendichter , The check will also occur on class construction. For example:

>> types.core.IntracellularElectrode()

ans = 

  IntracellularElectrode with properties:

                  description: ''
                       device: []
                      cell_id: ''
                    filtering: ''
    initial_access_resistance: ''
                     location: ''
                   resistance: ''
                         seal: ''
                        slice: ''

Warning: The following required properties are missing for instance for type "types.core.IntracellularElectrode":
    description
    device 

Note that the warning does not appear if the output is suppressed, e.g.

>> ielectrode = types.core.IntracellularElectrode();
>> 

and we could think about fine-tuning this, but I would suggest making that a separate issue. I would definitely go for warning on construction / error on export to be able to read files where these properties might be missing, but also as a more "gentle" way of teaching users what are required and not

[bendichter]

The warning is good, but I don't like that it is suppressed by the semicolon. I think most users will use a semicolon here and so they will miss the warning. It is a very important warning, so I don't want them to miss it. I was not aware of this behavior in MATLAB. Is this custom or standard warning behavior?