Overload isempty method for types.untyped.Set (Issue #561)

ehennestad commented 2 weeks ago

Fix #561

Motivation

See issue #561

How to test the behavior?

>> emptySet = types.untyped.Set();
>> isempty(emptySet)

ans =

  logical

   1

Checklist

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

ehennestad commented 2 weeks ago

The following snippet causes a lot of tests to fail, but is not relevant anymore:

https://github.com/NeurodataWithoutBorders/matnwb/blob/3222f33258775c1836c0aee670b3be1c1f74531b/%2Bio/parseGroup.m#L66-L70

The code was embedded in this context:

    %immediately elide prefix all property names with this but only if there are
    %no typed objects in it.
    propnames = keys(props);
    if hasTypes
        parsed = containers.Map({root}, {props});
    else
        parsed = containers.Map;
        for i=1:length(propnames)
            pnm = propnames{i};
            p = props(pnm);
            parsed([root '_' pnm]) = p;
        end
    end

    if isempty(parsed)
        %special case where a directory is simply empty.  Return itself but
        %empty
        parsed(root) = [];
    end

It was possible to create a containers.Map object named parsed that was empty, but parsed was later changed (see below) to be a types.untyped.Set which per definition (ref this PR) can not be empty (unless specifically created with types.untyped.Set.empty)

https://github.com/NeurodataWithoutBorders/matnwb/blob/5aa27d92cba1b9136612cc8ab80d0c1791975997/%2Bio/parseGroup.m#L56-L63

Code coverage also shows that lines are not covered by tests. Will remove this part

codecov[bot] commented 2 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 90.95%. Comparing base (5aa27d9) to head (6bcafd2).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #616 +/- ## ========================================== + Coverage 90.91% 90.95% +0.04% ========================================== Files 107 107 Lines 4753 4752 -1 ========================================== + Hits 4321 4322 +1 + Misses 432 430 -2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ehennestad commented 1 week ago

Current thinking: This should be not be merged as is. A types.untyped.Set is similar to a struct, containers.Map or dictionary in that it holds name-value pairs. Non of the builtin MATLAB types are empty if they have no name-value pairs, i.e s = struct is a scalar, and s = struct.empty is empty.

The same should be true for the Set.

Instead of redefining is empty, I think it would be better to not override size as is currently done, and also to change the custom display for a Set with no elements from: Empty Set to: Set with no elements or Set with no name, value pairs

NeurodataWithoutBorders / matnwb