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

Fix values provided by H5Grove #836

Merged
merged 7 commits into from
Nov 3, 2021
Merged

Fix values provided by H5Grove #836

merged 7 commits into from
Nov 3, 2021

Conversation

axelboc
Copy link
Contributor

@axelboc axelboc commented Nov 3, 2021

  • Return scalar values instead of typed arrays of length 1.
  • Restore flattening of non-numeric arrays.
  • Return classic JS arrays instead of typed arrays until we decide whether/how to support typed arrays in the visualizations.
  • Assert values returned by providers against datasets type and shape to avoid future regressions - fix [types] Strengthen Value<D> type #824

Copy link
Member

@loichuder loichuder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a complexity layer that is missed by your assertions: the presence of selection in the params changes the shape of the array returned from the request. It can even return a scalar value instead of an array.

This means the checks of shapes (such as in h5grove provider and in assertDatasetValue) must be done according to the asked selection rather than considering the full dataset metadata. Same thing for the flattening.

@axelboc
Copy link
Contributor Author

axelboc commented Nov 3, 2021

Right, I need to re-add the flattenValue utility that we've removed. That being said, I don't think it ever supported the case of the selection of a scalar. When we ignore this edge case, assertDatasetValue works fine, as it always receives an array.

Since this edge case isn't something we encounter in the viewer, I don't really see the point of supporting it. At least not in this PR.

@axelboc axelboc requested a review from loichuder November 3, 2021 12:26
entity: Dataset<ArrayShape>,
selection?: string
) {
assertArray(value);
Copy link
Contributor Author

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).

Copy link
Member

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:

  • Here and in assertDatasetValue usage locations, we have access to selection. So we could assert according to the selection.
  • The type inference in useDatasetValue could make use of dimMapping to have a Value<D> corresponding to the gotten slice.

Anyway, this will do for now.

Copy link
Contributor Author

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 of selection.

If anything, it would make more sense to prevent useDatasetValue from being called with a dimMapping that only contains numbers.

Copy link
Contributor Author

@axelboc axelboc Nov 3, 2021

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 of getValue 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.

entity: Dataset<ArrayShape>,
selection?: string
) {
assertArray(value);
Copy link
Member

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:

  • Here and in assertDatasetValue usage locations, we have access to selection. So we could assert according to the selection.
  • The type inference in useDatasetValue could make use of dimMapping to have a Value<D> corresponding to the gotten slice.

Anyway, this will do for now.

@axelboc axelboc merged commit cf049cb into main Nov 3, 2021
@axelboc axelboc deleted the value-fix branch November 3, 2021 14:04
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 this pull request may close these issues.

[types] Strengthen Value<D> type
2 participants