HDF5 Data reader enhancements#2328
Draft
bvanessen wants to merge 3 commits intoLBANN:v1.x-developfrom
Draft
Conversation
benson31
reviewed
Sep 26, 2023
Comment on lines
363
to
365
| int n_elts = node[pathname].dtype().number_of_elements(); | ||
| conduit::int64_array data_array_dims = metadata[HDF5_METADATA_KEY_DIMS].value(); | ||
| auto expected_n_elts = data_array_prod(data_array_dims); |
Collaborator
There was a problem hiding this comment.
Suggested change
| int n_elts = node[pathname].dtype().number_of_elements(); | |
| conduit::int64_array data_array_dims = metadata[HDF5_METADATA_KEY_DIMS].value(); | |
| auto expected_n_elts = data_array_prod(data_array_dims); | |
| int const n_elts = node[pathname].dtype().number_of_elements(); | |
| conduit::int64_array const data_array_dims = metadata[HDF5_METADATA_KEY_DIMS].value(); | |
| auto const expected_n_elts = data_array_prod(data_array_dims); |
Comment on lines
29
to
45
| namespace conduit { | ||
|
|
||
| template <typename T> | ||
| T | ||
| data_array_prod(DataArray<T> a) | ||
| { | ||
| T res = 1; | ||
| for(index_t i = 0; i < a.number_of_elements(); i++) | ||
| { | ||
| const T &val = a.element(i); | ||
| res *= val; | ||
| } | ||
|
|
||
| return res; | ||
| } | ||
|
|
||
| } // conduit |
Collaborator
There was a problem hiding this comment.
Suggested change
| namespace conduit { | |
| template <typename T> | |
| T | |
| data_array_prod(DataArray<T> a) | |
| { | |
| T res = 1; | |
| for(index_t i = 0; i < a.number_of_elements(); i++) | |
| { | |
| const T &val = a.element(i); | |
| res *= val; | |
| } | |
| return res; | |
| } | |
| } // conduit | |
| namespace lbann { | |
| template <typename T> | |
| T | |
| data_array_prod(DataArray<T> const& a) | |
| { | |
| T res = 1; | |
| for(index_t i = 0; i < a.number_of_elements(); i++) | |
| { | |
| const T &val = a.element(i); | |
| res *= val; | |
| } | |
| return res; | |
| } | |
| } // namespace lbann |
IMO we shouldn't be extending their namespace; just put it in lbann. Also, conduit::DataArray doesn't own any data, so it's just a "shallow copy" (pointer and size), but the "expected" API would probably be pass-by-const-reference. (I'm moderately surprised that they don't expose begin() and end() functions (and suitable iterators) for this container!)
Comment on lines
368
to
375
| LBANN_WARNING("Ingesting sample field ", | ||
| pathname, | ||
| " for sample ", | ||
| sample_name, | ||
| " where the dimensions in the metadata don't match the actual field: ", | ||
| expected_n_elts, | ||
| " != ", | ||
| n_elts); |
Collaborator
There was a problem hiding this comment.
Might be good to highlight in the output which is the observed vs the expected value -- shouldn't have to look in source code to decipher warnings...
each field actually matches the dimensions of the data fields. Added a helper function for conduit to allow the calculation of a product of a data array's elements.
903e928 to
90cb6e7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Added a check in the HDF5 data reader to check that the metadata for
each field actually matches the dimensions of the data fields.
Added a helper function for conduit to allow the calculation of a product of a data array's elements.