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.

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

595 Allow using empty indices for datastub #605

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

Conversation

ehennestad
Copy link
Collaborator

@ehennestad ehennestad commented Oct 31, 2024

Fix #595

Motivation

Fix issue.

How to test the behavior?

See issue #595

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?

@ehennestad
Copy link
Collaborator Author

@lawrence-mbf I would value your input here.

The proposed fix works in the specific situation, but if you have ideas on how to improve this, or if there are things I have overlooked please let me know.

Copy link

codecov bot commented Oct 31, 2024

Codecov Report

Attention: Patch coverage is 15.38462% with 11 lines in your changes missing coverage. Please review.

Project coverage is 90.42%. Comparing base (bc4b87d) to head (1b4dbfb).

Files with missing lines Patch % Lines
+types/+untyped/@DataStub/load_mat_style.m 15.38% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #605      +/-   ##
==========================================
- Coverage   90.63%   90.42%   -0.22%     
==========================================
  Files         106      106              
  Lines        4686     4698      +12     
==========================================
+ Hits         4247     4248       +1     
- Misses        439      450      +11     
Flag Coverage Δ
?

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

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

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.

@@ -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

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.

[Bug]: DataStub errors out if trying to retrieve data with empty indices
2 participants