-
Notifications
You must be signed in to change notification settings - Fork 58
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
Legate HDF5 using kerchunk #222
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor suggestions and a few queries.
legate/cpp/tile_io.cpp
Outdated
for (int32_t dim = 0; dim < DIM; ++dim) { | ||
idx = idx * extents[dim] + point[dim] - lo[dim]; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe translate to zero-origin up front? (Does this do the right thing if point_dp
is not in the box described by lo
and hi
?)
legate::Point<DIM> point = point_dp - lo_dp;
...
for (...) {
idx = idx * extents[dim] + points[dim];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
point_dp
must be within lo
and hi
. Since they come from context.get_task_index()
and context.get_launch_domain()
directly , I don't we need a check here.
…o legate_kerchunk
Co-authored-by: Lawrence Mitchell <[email protected]>
Thanks @wence- , I think I have addressed all of your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @madsbk , one required correctness-checking change and I've now had a chance to go through the C++ task description and have one more query there.
Co-authored-by: Lawrence Mitchell <[email protected]>
Thanks @wence-! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there was one "fixed" that you didn't push, but looks good, thanks
Thanks @wence- ! |
/merge |
Support reading of HDF5 files using kerchunk.
Limitations
Example