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

H5Dchunk_iter offset is not scaled by chunk dimensions #1419

Closed
wenjuno opened this issue Feb 3, 2022 · 5 comments · Fixed by #1969
Closed

H5Dchunk_iter offset is not scaled by chunk dimensions #1419

wenjuno opened this issue Feb 3, 2022 · 5 comments · Fixed by #1969

Comments

@wenjuno
Copy link

wenjuno commented Feb 3, 2022

I'm not very sure if this is intentional but I find it inconsistent with offset in H5Dget_chunk_info.
I think the relevant code for H5Dchunk_iter is this:

hdf5/src/H5Dchunk.c

Lines 7519 to 7520 in 1d59818

if ((ret_value = (data->op)(chunk_rec->scaled, chunk_rec->filter_mask, chunk_rec->chunk_addr,
chunk_rec->nbytes, data->op_data)) < 0)

Comparing to how the offset in H5Dget_chunk_info is calculated below, there are missing multipliers of dset->shared->layout.u.chunk.dim[ii]:

hdf5/src/H5Dchunk.c

Lines 7347 to 7349 in 1d59818

if (offset)
for (ii = 0; ii < udata.ndims; ii++)
offset[ii] = udata.scaled[ii] * dset->shared->layout.u.chunk.dim[ii];

In order to use H5Dchunk_iter I will need to apply the chunk dimensions multipliers myself. Is this a bug or a conscious design choice? Thanks.

@mkitti
Copy link
Contributor

mkitti commented Aug 4, 2022

This is an important inconsistency. @gauteh was this intended?

@gauteh
Copy link
Contributor

gauteh commented Aug 4, 2022

Hm, that was probably not intended: I can see in my library code that I scale it when this function is available, and that I don't scale when I use chunk_info directly.

I also noticed that in

*size = udata.nbytes;
the *offset seems to not be written to, that also seems like a bug?

Neither it seems that the offset is set here:

if (!different) {

@mkitti
Copy link
Contributor

mkitti commented Aug 4, 2022

the *offset seems to not be written to, that also seems like a bug?

You're looking at H5Dget_chunk_info_by_coord which takes offset as an input parameter.

https://docs.hdfgroup.org/hdf5/develop/group___h5_d.html#ga408a49c6ec59c5b65ce4c791f8d26cb0

@gauteh
Copy link
Contributor

gauteh commented Aug 4, 2022

Doh, of course.

@mkitti
Copy link
Contributor

mkitti commented Aug 4, 2022

I'll tackle if this unless someone else has started already.

@lrknox lrknox closed this as completed in 7127d89 Aug 7, 2022
mkitti added a commit to mkitti/hdf5 that referenced this issue Aug 8, 2022
…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
derobins added a commit that referenced this issue Aug 16, 2022
* 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 <[email protected]>
mkitti added a commit to mkitti/hdf5 that referenced this issue Dec 21, 2022
…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
mkitti added a commit to mkitti/hdf5 that referenced this issue Dec 21, 2022
derobins pushed a commit that referenced this issue Feb 21, 2023
* 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>
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 a pull request may close this issue.

3 participants