Add support for nD datasets in CompoundMatrix #1169
Conversation
packages/app/src/vis-packs/core/compound/CompoundMatrixVisContainer.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| const [slicedDims, slicedMapping] = useSlicedDimsAndMapping(dims, dimMapping); | ||
| const [mappedArray] = useMappedArray( | ||
| value.flat(1), |
There was a problem hiding this comment.
Given our dimMapping, value will always be a 1D array of tuples. So value.flat(1) is still valid to flatten the tuples.
| value: ArrayValue<PrintableType>; | ||
| dims: number[]; | ||
| dimMapping: DimensionMapping; | ||
| toolbarContainer: HTMLDivElement | undefined; | ||
| } |
There was a problem hiding this comment.
Looking at what is needed to add support for nD, I realized that there were a few props in MappedMatrixVis that were derived from other props:
selectioncan be derived fromdimMappingthroughgetSliceSelectiondimsis simplydataset.shape
Therefore, I think removing these props simplifies the component and reduces the risk of misuse.
There was a problem hiding this comment.
Hmm true, true, I was going to argue against calling getSliceSelection in MappedCompoundMatrixVis, but you're making an excellent point. getSliceSelection is cheap, so why not 🤷
Do you want to do this in the other containers/mapped vis, then?
There was a problem hiding this comment.
Ha, didn't realize it was the case for other containers. I will revert it for MatrixVis and do another PR to do it for all containers at once.
d1fae05 to
9576550
Compare
packages/app/src/vis-packs/core/compound/CompoundMatrixVisContainer.tsx
Outdated
Show resolved
Hide resolved
| value: ArrayValue<PrintableType>; | ||
| dims: number[]; | ||
| dimMapping: DimensionMapping; | ||
| toolbarContainer: HTMLDivElement | undefined; | ||
| } |
There was a problem hiding this comment.
Hmm true, true, I was going to argue against calling getSliceSelection in MappedCompoundMatrixVis, but you're making an excellent point. getSliceSelection is cheap, so why not 🤷
Do you want to do this in the other containers/mapped vis, then?
9576550 to
6b7e627
Compare
|
/approve |
Update Cypress reference snapshots
Follow-up of #1130
I also refactored a few things: I will underline them in the comments. Tell me if you agree.