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

HDF5Base can identify empty objects #330

Merged
merged 8 commits into from
Jun 5, 2023
Merged

Conversation

willGraham01
Copy link
Collaborator

@willGraham01 willGraham01 commented Jun 2, 2023

Context

Naturally, MATLAB does not save empty arrays or structs as objects with no elements or zero-size, instead it saves them as 2-by-1 arrays with 0's populating the data. This means that a simple comparison against the number of elements or members does not provide the correct information when attempting to determine whether an empty input has been passed.

MATLAB does however attach an attribute to any object that it marks as empty: MATLAB_empty. This uint8 is set to 1 if the object is indeed an empty array, so that is what we will use to distinguish.

Details

Adds the HDF5Base::flagged_MATLAB_empty method to replace the mxIsEmpty function, which a number of our classes depend on when reading from the input file.

Going forward, if the input generation is ever moved away from MATLAB too, a workaround will need to be decided to replace/handle .hdf5 files that are not created with MATLAB, as the analogue of this flag will not exist.

Testing

  • tdms/tests/unit/hdf5_io/test_hdf5_base.cpp added with extensive tests for the new flagged_MATLAB_empty method.
  • (The scripts that produce the) .mat and .hdf5 unit test data has been updated with the necessary additions

@willGraham01 willGraham01 force-pushed the hdf5-identify-empty-objects branch from e12e1f0 to 681554f Compare June 2, 2023 15:53
@codecov
Copy link

codecov bot commented Jun 2, 2023

Codecov Report

Patch coverage: 96% and no project coverage change.

Comparison is base (fdbfc93) 26% compared to head (cdc787d) 26%.

Additional details and impacted files
@@         Coverage Diff         @@
##           main   #330   +/-   ##
===================================
  Coverage    26%    26%           
===================================
  Files        69     69           
  Lines      3674   3699   +25     
===================================
+ Hits        952    976   +24     
- Misses     2722   2723    +1     
Impacted Files Coverage Δ
tdms/include/hdf5_io/hdf5_base.h 0% <ø> (ø)
tdms/src/hdf5_io/hdf5_base.cpp 96% <96%> (-<1%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@willGraham01 willGraham01 requested a review from samcunliffe June 5, 2023 08:42
Copy link
Member

@samcunliffe samcunliffe left a comment

Choose a reason for hiding this comment

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

🍉

No substantial comments. You can take or leave these...

tdms/include/hdf5_io/hdf5_base.h Outdated Show resolved Hide resolved
// the object.
return file_->isHdf5(filename_);
// return file_->isAccessible(filename_) && file_->isHdf5(filename_);
}

bool HDF5Base::flagged_MATLAB_empty(const std::string &object_path) const {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe is_flagged_as_MATLAB_empty(object_path)?
I don't insist though.

tdms/src/hdf5_io/hdf5_base.cpp Outdated Show resolved Hide resolved
tdms/src/hdf5_io/hdf5_base.cpp Outdated Show resolved Hide resolved
# Create test data to read in
consecutive_numbers = np.arange(0, 12, dtype=float)

# Create a group under root
read_in_test = file.require_group("read_in_test")
Copy link
Member

Choose a reason for hiding this comment

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

This, however, does need a comment because it's weird to me that require_group creates a group.

Copy link
Member

Choose a reason for hiding this comment

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

#libhdf5-world-problems

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's h5py for you 😅

@samcunliffe samcunliffe added this pull request to the merge queue Jun 5, 2023
Merged via the queue into main with commit c3cb66a Jun 5, 2023
@samcunliffe samcunliffe deleted the hdf5-identify-empty-objects branch June 5, 2023 13:43
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.

2 participants