HDFGroup / vol-rest

HDF5 REST VOL Connector
Other
5 stars 8 forks source link

rest_vol_dataset: (feature) Fixed memory-side hyperslab #58

Closed jwsblokland closed 11 months ago

jwsblokland commented 11 months ago

This is improved version of PR #50. Related PR #57. Most likely a combination of PR #57 and #58 will result in the best solution.

mattjala commented 11 months ago

The current tests don't cover any cases where a selection is non-contiguous, or where the value returned from RV_is_selection_contiguous is nonzero. Once those cases are tested, this should be good to merge.

jwsblokland commented 11 months ago

In my latest commit, I several improvements of the RV_dataspace_selection_is_contiguous() function. Now it captures also the errors correctly. Furthermore, I also implemented a memory-side hyperslab test in which the hyperslab is non-contiguous in memory.

jwsblokland commented 11 months ago

I have implemented an additional test in which the memory hyperslab is continuous and the memory offset is non-zero.

Furthermore, I have re-implemented my changes based on the latest master branch. The main reason for this, is that due to the recent added support of multi-datasets it was easier to reimplement it than rebasing my old branch to the latest master.

mattjala commented 11 months ago

Non-contiguous and nonzero offset cases look good. Should be good to merge once the point selection RV_dataspace_selection_is_contiguous issue is fixed.

jhendersonHDF commented 11 months ago

@jwsblokland Thanks for all your work on this PR! I think it's very close with just a few minor comments on improvements that could be made.

jwsblokland commented 11 months ago

@jhendersonHDF Similar as the improvement I made for ROS3, it was fun implementing this improvement. Again, I got a better understanding of HDF5 but now about dataspaces. I addressed your comments. At least that is my intent. Regarding the support of non-regular hyperslabs, I think it is better to create a separate PR for it. Reading your comment about it, it sounds like this is not trivial at all.