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.
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
Fix values provided by H5Grove #836
Fix values provided by H5Grove #836
Changes from all commits
afe2468
8c24b46
f3340bd
b641125
8cb6097
fa9708f
4399b7b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This will throw if we ask the API to select a scalar, so at least we'll know (in the unlikely event we write code that does this inadvertently).
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 understand that given our usecases, we will never get a scalar from an array dataset.
Still it bothers me to prevent it for the following reasons:
assertDatasetValue
usage locations, we have access toselection
. So we could assert according to theselection
.useDatasetValue
could make use ofdimMapping
to have aValue<D>
corresponding to the gotten slice.Anyway, this will do for now.
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.
Sure, it's all doable, but adding this level of type safety to
useDatasetValue
means that we then have to assert that the returned value is not scalar inside the relevant containers, which leads to exactly the same result as the assertion that the value returned by the provider is an array regardless ofselection
.If anything, it would make more sense to prevent
useDatasetValue
from being called with adimMapping
that only contains numbers.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.
Either way, we still have implicit coupling between
useDatasetValue
and the provider APIs'getValue
methods... Perhaps a better solution would be to rethink the signature ofgetValue
to make this coupling more explicit - e.g. by letting the method accept a dataset rather than an entity, and an optional dimension mapping array with at least one axis in it.