H5Dchunk_iter now passes offsets in units of dataset elements, fix #1419 #1969
Conversation
mkitti
requested review from
jhendersonHDF,
fortnern,
soumagne,
vchoi-hdfgroup,
ChristopherHogan,
gnuoyd,
derobins,
jrmainzer and
qkoziol
as code owners
|
We should probably update the documentation (H5Dpublic.h> to clarify that "logical coordinates of chunk" uses units of dataset elements |
|
I'll approve and merge this once the documentation is updated. |
Modified description for `H5Dchunk_iter`, `H5Dget_chunk_info`, and `H5Dget_chunk_info_by_coord` to the following * offset Logical position of the chunk’s first element in units of dataset elements * filter_mask Bitmask indicating the filters used when the chunk was written * size Chunk size in bytes, 0 if the chunk does not exist
|
We should regularize the description of the arguments. The descriptions of the arguments should be the same between Also I changed |
mkitti
changed the title
|
Looks good to me. Thanks! |
|
This also needs a note in RELEASE.txt, but I'll take care of that. |
|
Thank you for the reviews. I will leave this branch alone now, and begin moving these changes into 1.12 via #1970 . |
…t elements, fix HDFGroup#1419 (HDFGroup#1969) * H5Dchunk_iter now passes chunk dimension scaled offsets, fix HDFGroup#1419 * Update docs for H5Dchunk_iter, H5Dget_chunk_info* Modified description for `H5Dchunk_iter`, `H5Dget_chunk_info`, and `H5Dget_chunk_info_by_coord` to the following * offset Logical position of the chunk’s first element in units of dataset elements * filter_mask Bitmask indicating the filters used when the chunk was written * size Chunk size in bytes, 0 if the chunk does not exist
* Revert "Revert H5Dchunk_iter changes in hdf5_1_12_1 (#733)" This reverts commit 10abe9a. * Apply clang-format * Reincorporate spelling fix from #1166 * Incorporate H5Dchunk_iter changes from #161 * Backport to 1.12: Adds a quick for for some egregious chunk_info badness (#722) * Backport to 1.12: Converts testhdf5 macros to h5test macros in chunk_info.c (#1820) The two macro schemes were not designed to work together. Also quiets some MSVC warnings about comparing pointers and integers. * Backport to 1.12: H5Dchunk_iter now passes offsets in units of dataset elements, fix #1419 (#1969) * H5Dchunk_iter now passes chunk dimension scaled offsets, fix #1419 * Update docs for H5Dchunk_iter, H5Dget_chunk_info* Modified description for `H5Dchunk_iter`, `H5Dget_chunk_info`, and `H5Dget_chunk_info_by_coord` to the following * offset Logical position of the chunk’s first element in units of dataset elements * filter_mask Bitmask indicating the filters used when the chunk was written * size Chunk size in bytes, 0 if the chunk does not exist * Sync H5Dchunk_iter documentation with develop * Remove H5VL_DATASET_WAIT references from 1.12 * Backport to 1.12 #161, #1474, review comments Co-authored-by: Dana Robinson <43805+derobins@users.noreply.github.com>
…FGroup#1419 (HDFGroup#1969) * H5Dchunk_iter now passes chunk dimension scaled offsets, fix HDFGroup#1419 * Update docs for H5Dchunk_iter, H5Dget_chunk_info* Modified description for `H5Dchunk_iter`, `H5Dget_chunk_info`, and `H5Dget_chunk_info_by_coord` to the following * offset Logical position of the chunk’s first element in units of dataset elements * filter_mask Bitmask indicating the filters used when the chunk was written * size Chunk size in bytes, 0 if the chunk does not exist
| unsigned ii; /* Match H5O_layout_chunk_t.ndims */ | ||
|
|
||
| /* Similar to H5D__get_chunk_info */ | ||
| for (ii = 0; ii < chunk->ndims; ii++) |
There was a problem hiding this comment.
I think this is off by one? The ndims here is 1 larger than the rank I set. I also see in other places, layout->ndims is decreased by 1 to determine the number of dimensions.
Lines 1095 to 1100 in 509fe96
There was a problem hiding this comment.
If you add an assertion here and run this test, it's easy to see that chunk->ndims is 3 but we clearly expect the offset array to have two valid elements.
There was a problem hiding this comment.
You are correct that there are ndims - 1 dimensions, but this is an internal implementation detail of the library. That specific loop comes from H5D__get_chunk_info.
What is the actual problem though? At worse, we do one more multiplication than needed while avoiding a subtraction and another variable on the stack.
* Backport H5Dchunk_iter to 1.10 branch * Add some accessory files, test still needs work * Apply proper formatting, and fix compile errors * Remove const from H5D__chunk_iter as per #1700 * Align arg types of H5D_chunk_iter_op_t with H5Dget_chunk_info (#2074) * Align arg types of H5D_chunk_iter_op_t with H5Dget_chunk_info * Modify chunk_info test to for unsigned / hsize_t types * Fix types in test * Add test_basic_query, helper functions to test/chunk_info.c 1_10 * H5Dchunk_iter now passes offsets in units of dataset elements, fix #1419 (#1969) * H5Dchunk_iter now passes chunk dimension scaled offsets, fix #1419 * Update docs for H5Dchunk_iter, H5Dget_chunk_info* Modified description for `H5Dchunk_iter`, `H5Dget_chunk_info`, and `H5Dget_chunk_info_by_coord` to the following * offset Logical position of the chunk’s first element in units of dataset elements * filter_mask Bitmask indicating the filters used when the chunk was written * size Chunk size in bytes, 0 if the chunk does not exist * Fix regression of #1419 * Add a note about return fail in 1.12 and older for invalid chunk index * Committing clang-format changes * Run clang-format on test/chunk_info.c --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
This fixes #1419 by adding a
H5O_layout_chunk_tpointer toH5D_chunk_iter_ud_tand using thedimandndimfields.This uses an statically sized array
hsize_t offset[H5O_LAYOUT_NDIMS]which is passed to the user callback function.test/chunk_info.cis modified to match the changes and is now consistent withH5Dget_chunk_info.Tasks