Skip to content

Fetch int-64 values from h5grove as binary to maintain precision#1747

Merged
axelboc merged 1 commit intomainfrom
h5grove-bigint
Feb 3, 2025
Merged

Fetch int-64 values from h5grove as binary to maintain precision#1747
axelboc merged 1 commit intomainfrom
h5grove-bigint

Conversation

@axelboc
Copy link
Contributor

@axelboc axelboc commented Jan 30, 2025

Follows #1745 and closes #1679

return Int32Array;
case 64: // No support for 64-bit integer values in JS
case 64:
return undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Why not returning BigInt64Array here ?

Copy link
Contributor Author

@axelboc axelboc Feb 3, 2025

Choose a reason for hiding this comment

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

Sorry, should have explained: for the same reason I have a separate type union, a separate type guard, etc. — when both kinds of typed arrays are mixed together in a union, TypeScript goes wild:

  • You can't invoke the Big(U)Int64Array constructors with number[], only with bigint[] (so new IntOrBigIntTypedArray(arr as number [] | bigint[]) doesn't type-check)
  • You can't loop through the array, since number[] and bigint[] aren't compatible.

Having a single function worked fine in h5grove-api since we invoke the typed array constructor with a buffer. However, it failed in useIgnoreFillValue, where we pass a number[]:

  const DTypedArray = typedArrayFromDType(dataset.type);

  // Cast fillValue in the type of the dataset values to be able to use `===` for the comparison
  const fillValue =
    DTypedArray && typeof wrappedFillValue[0] === 'number'
      ? new DTypedArray(wrappedFillValue as number[])[0]
      : undefined;

Arguably, perhaps useIgnoreValue should be reworked to support bigints as well... Seemed like a can of worms, though, so I went with a compromise.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, ok I see.

@axelboc axelboc force-pushed the bigint-new branch 4 times, most recently from 6852665 to 0748093 Compare February 3, 2025 10:03
Base automatically changed from bigint-new to main February 3, 2025 10:34
@axelboc axelboc merged commit 85e418b into main Feb 3, 2025
8 checks passed
@axelboc axelboc deleted the h5grove-bigint branch February 3, 2025 14:52
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.

64-bit integers not shown correctly in h5web display

2 participants