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

Transform null in NaN in h5grove and Jupyter providers #776

Closed
wants to merge 1 commit into from

Conversation

loichuder
Copy link
Member

Fixes Jupyter/h5grove issues reported in #641 (comment):

❌ The null value gets interpreted as 0 in most numeric contexts: value display in the tooltip, domain computation, colormap mapping, MatrixVis...
❌ Sole exception of the rule above: ScalarVis throws with val is null

@loichuder loichuder requested a review from axelboc August 6, 2021 13:40
@axelboc
Copy link
Contributor

axelboc commented Aug 9, 2021

Nice and simple implementation, but three concerns (which I'm sure you're aware of):

  1. Can we get a null value on purpose sometimes? Isn't that what type H5_EMPTY is supposed to be serialised as? Unlikely to occur in the wild and we don't support it anyway, I know, but still... If we merge this JSON parsing algorithm, we close the door on this for now.
  2. Obviously null could mean Infinity rather than NaN.
  3. Obviously we're doing an extra loop over the data, which isn't ideal.

The second concern is particularly critical to me, since we could potentially "lie" to the user by showing them NaN instead of Infinity. Currently we already lie by showing 0, I know, but I guess what I'm saying is that we're not really improving the situation here... This, combined with the other two concerns, makes me wonder whether this temporary fix is worth making, or whether we should rather just wait for binary support.

@loichuder
Copy link
Member Author

loichuder commented Aug 9, 2021

Nice and simple implementation, but three concerns (which I'm sure you're aware of):

1. Can we get a `null` value on purpose sometimes? Isn't that what type `H5_EMPTY` is supposed to be serialised as? Unlikely to occur in the wild and we don't support it anyway, I know, but still... If we merge this JSON parsing algorithm, we close the door on this for now.

True. I completely missed H5_EMPTY... So fair, there is no way to distinguish between the value of H5_EMPTY and the value of a scalar being NaN/Infinity except if we look at the metadata. This might be our only way out.

2. Obviously `null` could mean `Infinity` rather than `NaN`.

Yes but again, this is not something we can solve on the front-end for now. I thought that displaying NaN instead of 0 for Infinity was the lesser evil.

3. Obviously we're doing an extra loop over the data, which isn't ideal.

I wholeheartedly agree. A quick search shows that reviver has a considerable impact on the JSON.parse performance. But again, there is no way around it when considering only the front-end.

The second concern is particularly critical to me, since we could potentially "lie" to the user by showing them NaN instead of Infinity. Currently we already lie by showing 0, I know, but I guess what I'm saying is that we're not really improving the situation here...

Well, I think NaN is way more common that Infinity which could make us "say the truth" in 90% of the cases (personal ballpark estimate). From my developer point of view, replacing null by Number.NaN also solves the TS typings. NdArray<number> and PrintableType are actually wrong currently as they don't take null into account.

This, combined with the other two concerns, makes me wonder whether this temporary fix is worth making, or whether we should rather just wait for binary support.

I made some tests that we need to discuss but binary support might not come that fast 😹

Copy link
Contributor

@axelboc axelboc left a comment

Choose a reason for hiding this comment

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

Alright, let's do this then.

@loichuder
Copy link
Member Author

Closing for now as the fix may not be worth the performance impact of the reviver (yeah, I changed my mind 😹 ). This will probably need to be revisited later.

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.

2 participants