Skip to content

Conversation

@ehennestad
Copy link
Collaborator

@ehennestad ehennestad commented Oct 29, 2025

Motivation

Fix wrong assumption in PR:

That PR changed DataStub to resolve data type and size once, and keep values cached. However a dataset represented by a DataStub can change size if expanded via the DataPipe/BoundPipe class. This PR ensures the size of a DataStub is updated whenever a dataset is expanded via a DataPipe object.

This PR also includes a refactor of DataStub, adding a maxSize property and improving the way a BoundPipe resolves its current_size and max_size.

How to test the behavior?

Show here how to reproduce the new behavior (can be a bug fix or a new feature)

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?

Introduces io.space.getSize to obtain current and maximum dataset dimensions from an HDF5 dataspace identifier. Handles dimension order conversion and unlimited dimension representation for MATLAB compatibility.
- Changed get_space to private method.
- Add maxDims property
- Added updateSize method, which can be accessed by BoundPipe, as stubs of BoundPipes are expandable and their size can change
- Improved subsref override
  - Support method calls with no output
  - Support compound indexing
Add test for nested data indexing
- Update DataStub size when dataset is appended/expanded
- Delegate detection of current size and max size to datastub
Improve fixture teardown
@codecov
Copy link

codecov bot commented Oct 29, 2025

Codecov Report

❌ Patch coverage is 96.29630% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 95.60%. Comparing base (42fe0f1) to head (4173d3b).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
+types/+untyped/@DataStub/DataStub.m 94.44% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #766   +/-   ##
=======================================
  Coverage   95.59%   95.60%           
=======================================
  Files         182      183    +1     
  Lines        6380     6388    +8     
=======================================
+ Hits         6099     6107    +8     
  Misses        281      281           

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ehennestad ehennestad requested a review from Copilot October 29, 2025 22:10
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the DataStub class to add a maxDims property and introduces a new utility function io.space.getSize for retrieving dataset dimensions. The changes improve code organization by centralizing dimension retrieval logic and enabling BoundPipe to access maximum dimensions through DataStub.

  • Introduced maxDims dependent property to track maximum dataset dimensions
  • Created io.space.getSize utility function to consolidate dimension retrieval logic
  • Updated subsref to support multiple output arguments for proper nargout behavior
  • Added numArgumentsFromSubscript method to handle compound data type indexing

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
+types/+untyped/@DataStub/DataStub.m Added maxDims property, refactored dimension retrieval to use io.space.getSize, moved get_space to private methods, updated subsref for varargout support, added updateSize and numArgumentsFromSubscript methods
+types/+untyped/+datapipe/BoundPipe.m Simplified constructor to use DataStub.maxDims, added updateSize call after append operation, added type validation for stub property
+tests/+unit/dataStubTest.m Moved test schema generation to TestClassSetup for efficiency, added new test for nested data indexing
+tests/+fixtures/ExtensionGenerationFixture.m Added existence checks before deleting directories and files to prevent errors
+io/+space/getSize.m New utility function that encapsulates HDF5 dimension retrieval and conversion logic

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Simplifies the logic in the dims and maxDims property getters by calling updateSize instead of duplicating code. Also updates the access level for updateSize to allow access from DataStub and BoundPipe classes.
@ehennestad ehennestad marked this pull request as ready for review October 29, 2025 22:19
@ehennestad ehennestad requested a review from bendichter October 29, 2025 22:34
bendichter
bendichter previously approved these changes Oct 30, 2025
Fix failing test
@ehennestad ehennestad requested a review from bendichter October 30, 2025 18:52
@ehennestad ehennestad merged commit 8d5173c into main Oct 30, 2025
18 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