Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

595 Allow using empty indices for datastub #605

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

595 Allow using empty indices for datastub #605

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fc9ca2a
Coarse fix for #595
ehennestad Sep 14, 2024
1b4dbfb
Merge branch 'master' into 595-use-empty-indices-for-datastub
ehennestad Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion +types/+untyped/@DataStub/load_mat_style.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,14 @@
, iDimension, dimensionSize);
end

if isscalar(userSelection) && ~ischar(userSelection{1})
if isscalar(userSelection) && isempty(userSelection{1})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conditional adds support for this syntax:

ds = types.untyped.DataStub(...);
emptytype = ds([]);

and

emptytype = ds('');

But not

emptytype = ds();

Is that the intention here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this should be updated.
emptytype = ds([]); should be supported, but neither emptytype = ds(''); or emptytype = ds(); should be

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on MATLAB's builtin behavior, it should perhaps be kept the way it is:

>> s = struct('a', num2cell(1:10))

s = 

  1×10 struct array with fields:

    a

>> s([])

ans = 

  0×0 empty struct array with fields:

    a

>> s('')

ans = 

  0×0 empty struct array with fields:

    a

>> s()

ans = 

  1×10 struct array with fields:

    a

% If userselection (indices) is empty, get the first element of this
% DataStub and try to return an empty representation of that type.
data = obj.load_mat_style(1);
data = getEmptyRepresentation(data);
return

Check warning on line 42 in +types/+untyped/@DataStub/load_mat_style.m

View check run for this annotation

Codecov / codecov/patch

+types/+untyped/@DataStub/load_mat_style.m#L40-L42 

Added lines #L40 - L42 were not covered by tests

elseif isscalar(userSelection) && ~ischar(userSelection{1})
% linear index into the fast dimension.
orderedSelection = unique(userSelection{1});

Expand Down Expand Up @@ -187,4 +194,19 @@
indexKeyIndex(indexKeyIndexNextIndex) = indexKeyIndex(indexKeyIndexNextIndex) + 1;
indexKeyIndex((indexKeyIndexNextIndex+1):end) = 1;
end
end


function emptyInstance = getEmptyRepresentation(nonEmptyInstance)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ehennestad Does this work with HDF5 reference types? Very clever, I did not know left-hand side colon assignment preserved types.

try
emptyInstance = nonEmptyInstance;
emptyInstance(:) = [];
catch ME
switch ME.identifier
lawrence-mbf marked this conversation as resolved.
Show resolved Hide resolved
case 'MATLAB:table:LinearSubscript'
emptyInstance(:, :) = [];

Check warning on line 207 in +types/+untyped/@DataStub/load_mat_style.m

View check run for this annotation

Codecov / codecov/patch

+types/+untyped/@DataStub/load_mat_style.m#L201-L207 

Added lines #L201 - L207 were not covered by tests
otherwise
error('Not implemented for value of type: "%s"', class(nonEmptyInstance))

Check warning on line 209 in +types/+untyped/@DataStub/load_mat_style.m

View check run for this annotation

Codecov / codecov/patch

+types/+untyped/@DataStub/load_mat_style.m#L209 

Added line #L209 was not covered by tests
end
end
end