Support visualizing nD boolean datasets as Line and Heatmap #1499
Conversation
There was a problem hiding this comment.
Honestly, this PR looks quite all right to me. Sure, it may not seem the prettiest since you need to propagate typing changes down the line, but there is no way around it. The alternatives you listed would have been even more verbose in terms of typings.
Perhaps you can introduce a NumericLikeType to go along with the new guards/assertions ?
|
Also, once there is a real usecase with |
|
I might just investigate option 3/4 a bit more, because with the current solution, when using |
b1f7d48 to
12c99a1
Compare
|
Option 3/4 wasn't as relevant after I realised in #1503 that In the end, I went with option 1.5... I convert the I also introduce a |
| return arr.map((val) => (val ? 1 : 0)); | ||
| } | ||
|
|
||
| return arr as NumArray; |
There was a problem hiding this comment.
Scratched my head trying to remove this as, but two problems make this difficult:
- The value array is separated from the dataset object that describes the type, so narrowing the type of the dataset object does not help narrow the type of the value array...
ArrayValue<NumericType | BooleanType>resolves to(number | boolean)[] | TypedArray. I actually found a way to getnumber[] | boolean[] | TypedArraybut it broke a bunch of code related to compound datasets ... and it makes sense for compound value arrays to have type(number | string | ...)[] | TypedArray).
There was a problem hiding this comment.
Yeah. I have the feeling there is always a way to remove as but sometimes it is not worth the complexity.
| @@ -30,7 +30,7 @@ interface Props { | |||
| aspect?: Aspect; | |||
| showGrid?: boolean; | |||
| title?: string; | |||
| dtype?: NumericType; | |||
| dtype?: NumericLikeType; | |||
There was a problem hiding this comment.
Not a fan of @h5web/lib knowing about NumericLikeType (or any other DType). I'm thinking I might change this to string and move the call to formatNumLikeType into the mapped components at some point.
|
/approve |
| return arr.map((val) => (val ? 1 : 0)); | ||
| } | ||
|
|
||
| return arr as NumArray; |
There was a problem hiding this comment.
Yeah. I have the feeling there is always a way to remove as but sometimes it is not worth the complexity.
Update Cypress reference snapshots
|
/approve |
Co-authored-by: axelboc <axelboc@users.noreply.github.com>
Proposal implementation for #1498 with some unresolved questions.EDIT: First part of #1498 (booleans — enums will come later).
I had a couple of options:
useDomainhandle boolean arrays (to return[0, 1]obviously) – apart from that, it turns out thatDataCurvehandlesbooleanarray out of the box (though taking advantage of this may not be such a great idea... 🤷 😄)Option 2 seemed to complexify a bunch lot of code in terms of typing. Option 1 obviously requires mapping through the
valuesarray, but seems like a good compromise in terms of complexity (even though it's not trivial either in terms of typing – see below.)A third option could be to let the data provider convert the boolean values to numbers when visualizing as line/heatmap; or maybe even to request a numeric array straight from the back-end.
Either way, there's a bit of unresolved complexity when it comes to the
Valuetype (i.e. what shouldValue<Dataset<ArrayShape, NumericType | BooleanType>>resolve to?), since for the matrix vis we want actual booleans (boolean[]), while for the line/heatmap vis, we want eitherTypedArray | number[] | boolean[](and not(number | boolean)[]!) for option 1 orTypedArray | number[]for option 2/3/4.Other unresolved question: the issue mentions enums, which makes sense since (if I recall) booleans are stored as enums in HDF5. However, we do treat them separately in H5Web. Perhaps that wasn't such a good idea? 🤷