Conversation
355d6ab to
3a3c561
Compare
|
/approve |
|
|
||
| default: | ||
| return (val) => { | ||
| return typeof val === 'bigint' ? val.toString() : formatter(val); |
There was a problem hiding this comment.
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.
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.
Refactoring done in 0748093
I've split createNumericFormatter into two:
createFloatFormatterwhich 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
|
|
||
| default: | ||
| return (val) => { | ||
| return typeof val === 'bigint' ? val.toString() : formatter(val); |
There was a problem hiding this comment.
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
bigintvalues 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 thebigintvalues reach the Scalar, Matrix and Compound visualizations without any loss of precision.This addresses most of #1679. (I'll close it once
H5GroveProvideralso returns bigints.)The preparatory work I did in #1736, #1737, #1738, #1739 and #1743 bore fruit. I didn't end up needing a separate
BigIntegerTypeor 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 typeIntegerTypeinstead ofUnknownTypeH5WasmApino longer converts bigints to numbersScalarValue<NumericType>is nownumber | bigintArrayValue<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/libdoesn't support bigints, those arrays also have to be converted inside mapped components withuseToNumArrayanduseToNumArrays.How I got everything to work
What mattered the most to not break the entire codebase was to ensure that
NumArraydidn't go fromnumber[]| TypedArraytonumber[] | bigint[] | TypedArray | BigIntTypedArray, asNumArrayis used throughout@h5web/lib.The passage between
Value<Dataset<Shape, DType>>andNumArrayhappens in the mapped components, so declaring the props of those components withArrayValue<NumericArray>instead ofNumArrayin #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,
EnumTypeandBoolTypecan have a 64-bitIntegerTypeas base type). This is enforced by theScalarValueandArrayValuetypes and by the corresponding assertion functions.Screenshots