Skip to content

Conversation

@ehennestad
Copy link
Collaborator

@ehennestad ehennestad commented Nov 11, 2024

Motivation

The following lines in io.writeCompound always evaluates to false, because the value in the field of the data structure is per definition a logical array. It also evaluates to a scalar, so it will not work if the field value is a logical vector.

boolNames = names(strcmp(classes, 'logical'));
for iField = 1:length(boolNames)
data.(boolNames{iField}) = strcmp('TRUE', data.(boolNames{iField}));
end

After fixing this, (for some reason which I don't know/understand) the respective part of parseCompound must also be updated:

logicalFieldName = fieldName(isLogicalType);
for iFieldName = 1:length(logicalFieldName)
name = logicalFieldName{iFieldName};
data.(name) = strcmp('TRUE', data.(name));
end

The expected behavior (in matnwb) when writing logicals via the int8 data type is that it will create an int8 enum type in the h5 file with values equal to TRUE or FALSE. However, when reading values back, the values are of type int8. The actual tid of the data (h5 type id) indicates that the data should be an "enum boolean", but the values are not.

This might have something to do how the compound data type is set up in h5, but I do not know the hdf5 library, so I can't judge if the following should work or not:

tid = H5T.create('H5T_COMPOUND', sum(sizes));
for i=1:length(names)
%insert columns into compound type
H5T.insert(tid, names{i}, sum(sizes(1:i-1)), tids{i});
end

The proposed fix does work in the unit test, and will also work if data would be read in as a "boolean enum" (TRUE/FALSE) values.

How to test the behavior?

run unit tests in tests.unit.io.WriteTest

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
Copy link

codecov bot commented Nov 11, 2024

Codecov Report

Attention: Patch coverage is 33.33333% with 6 lines in your changes missing coverage. Please review.

Project coverage is 90.81%. Comparing base (d970ef1) to head (0749a6a).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
+io/parseCompound.m 40.00% 3 Missing ⚠️
+io/writeCompound.m 25.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #620      +/-   ##
==========================================
+ Coverage   90.80%   90.81%   +0.01%     
==========================================
  Files         107      107              
  Lines        4774     4781       +7     
==========================================
+ Hits         4335     4342       +7     
  Misses        439      439              

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

@ehennestad ehennestad requested review from bendichter and lawrence-mbf and removed request for lawrence-mbf November 11, 2024 12:27
@bendichter bendichter merged commit 7caa336 into master Nov 14, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants