-
Notifications
You must be signed in to change notification settings - Fork 21
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
Allow providers to return bigints #1745
Conversation
355d6ab
to
3a3c561
Compare
/approve |
|
||
default: | ||
return (val) => { | ||
return typeof val === 'bigint' ? val.toString() : formatter(val); |
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've pushed a bug fix here. Int-64 values provided as number
were being cast to string instead of being formatted with D3.
I'm not 100% sure which is the result we want, actually - e.g. for fixed-point notation, do we want 1.000
or 1
? I assumed 1
, since decimals don't matter, so maybe I should revert and keep the toString()
? In that case, maybe toString()
should be the default formatting for all integers with auto and fixed-point notation?
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.
Yeah, it does not make much sense to do more than converting to strings for integers. toString
should be enough.
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.
Refactoring done in 0748093
I've split createNumericFormatter
into two:
createFloatFormatter
which works as beforecreateIntegerFormatter
:- with "auto" and "fixed-point" notation: casts numbers/bigints to string with
toString()
- with "scientific" notation: casts bigints to numbers with
Number
, and then formats numbers with D3 (same format as before:'.3e'
)
- with "auto" and "fixed-point" notation: casts numbers/bigints to string with
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.
👏 💪
|
||
default: | ||
return (val) => { | ||
return typeof val === 'bigint' ? val.toString() : formatter(val); |
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.
Yeah, it does not make much sense to do more than converting to strings for integers. toString
should be enough.
d507cfe
to
6852665
Compare
The mock and h5wasm providers now return
bigint
values for int-64 datasets (including compounds with int-64 fields). They no longer have to cast int-64 integers to JS numbers (i.e. float 64), which means that thebigint
values reach the Scalar, Matrix and Compound visualizations without any loss of precision.This addresses most of #1679. (I'll close it once
H5GroveProvider
also returns bigints.)The preparatory work I did in #1736, #1737, #1738, #1739 and #1743 bore fruit. I didn't end up needing a separate
BigIntegerType
or even the "generic solution" explained in #1679 (comment). (This generic solution was flawed anyway: allowing providers to declare which value type they want to return for which dataset shape/type is useless, since the viewer wants to know which value types any provider may return, so either way we end up with a type union.)Summary of the changes
1n
) are now "guessed" to have typeIntegerType
instead ofUnknownType
H5WasmApi
no longer converts bigints to numbersScalarValue<NumericType>
is nownumber | bigint
ArrayValue<NumericType>
is nowTypedArray | BigIntTypedArray | number[] | bigint[]
d3-format
).useToNumArray
, which was already responsible for converting booleans to numbers.ArrayValue<NumericType>
), and since D3 and most of our code in@h5web/lib
doesn't support bigints, those arrays also have to be converted inside mapped components withuseToNumArray
anduseToNumArrays
.How I got everything to work
What mattered the most to not break the entire codebase was to ensure that
NumArray
didn't go fromnumber[]| TypedArray
tonumber[] | bigint[] | TypedArray | BigIntTypedArray
, asNumArray
is used throughout@h5web/lib
.The passage between
Value<Dataset<Shape, DType>>
andNumArray
happens in the mapped components, so declaring the props of those components withArrayValue<NumericArray>
instead ofNumArray
in #1739 was key.Another trick that makes the implementation a lot simpler in this PR is to ensure that enum and boolean datasets cannot be provided as bigints but only as numbers (even though technically,
EnumType
andBoolType
can have a 64-bitIntegerType
as base type). This is enforced by theScalarValue
andArrayValue
types and by the corresponding assertion functions.Screenshots