Skip to content
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

Collection thumbs #125

Closed
wants to merge 11 commits into from
Closed

Conversation

will-moore
Copy link
Collaborator

@will-moore will-moore commented Oct 5, 2021

This is not even a draft PR, but just me learning and exploring options for viewing a collection. (See #123 and #124).
Very rough - barely working at this stage; just pushing as part of the discussion...

But locally I managed to view this collection https://minio-dev.openmicroscopy.org/idr/v0.3/datasets/idr0062/8dc4707/7754.zarr
which has only 11 images but they are different sizes and can't be viewed by #124
(From Dataset at http://idr.openmicroscopy.org/webclient/?show=dataset-7754)

Screenshot 2021-10-12 at 13 03 01

@will-moore will-moore closed this Jan 5, 2022
@will-moore
Copy link
Collaborator Author

Re-opening this PR with a view to updating it to support "bioformats2raw.layout": 3 collections of images as described at ome/ngff#112.

@will-moore will-moore reopened this May 9, 2022
@will-moore
Copy link
Collaborator Author

Something has probably changed since I had this running previously, but now I can't get Thumbnails to render

Screenshot 2022-05-10 at 10 49 33

And also hit a blocker trying to fix TypeScript

$ npm run check

> @hms-dbmi/[email protected] check
> tsc

src/components/NgffCollection/ImageThumbnail.tsx:103:7 - error TS2345: Argument of type '{ loader: ZarrPixelSource<string[]>; loaderSelection: (number | undefined)[][]; colorValues: number[][]; sliderValues: number[][]; channelIsOn: boolean[]; }' is not assignable to parameter of type 'Viv<LayerProps, string[]>'.
  Object literal may only specify known properties, and 'loaderSelection' does not exist in type 'Viv<LayerProps, string[]>'.

103       loaderSelection: chunks,
          ~~~~~~~~~~~~~~~~~~~~~~~

src/vizarr.tsx:29:19 - error TS2345: Argument of type '(prevSourceInfo: WithId<SourceData>[]) => (WithId<SourceData> | { name?: string | undefined; images: any[]; id: string; })[]' is not assignable to parameter of type 'SetStateAction<WithId<SourceData>[]>'.
  Type '(prevSourceInfo: WithId<SourceData>[]) => (WithId<SourceData> | { name?: string | undefined; images: any[]; id: string; })[]' is not assignable to type '(prev: WithId<SourceData>[]) => WithId<SourceData>[]'.
    Type '(WithId<SourceData> | { name?: string | undefined; images: any[]; id: string; })[]' is not assignable to type 'WithId<SourceData>[]'.
      Type 'WithId<SourceData> | { name?: string | undefined; images: any[]; id: string; }' is not assignable to type 'WithId<SourceData>'.
        Type '{ name?: string | undefined; images: any[]; id: string; }' is not assignable to type 'WithId<SourceData>'.
          Type '{ name?: string | undefined; images: any[]; id: string; }' is missing the following properties from type 'SourceData': loader, channel_axis, colors, names, and 5 more.

29     setSourceInfo((prevSourceInfo) => {
                     ~~~~~~~~~~~~~~~~~~~~~

@manzt
Copy link
Member

manzt commented May 10, 2022

Should be able to have a look in the next day or so. My guess is that there are some changes to state organization that merging might have messed with.

@manzt
Copy link
Member

manzt commented May 27, 2022

Hi will, as for the second type error:

setSourceInfo((prevSourceInfo) => {

The issue is that the type gaurd:

   if ('images' in sourceData && sourceData["images"].length > 0) {
      // This is a Collection

Doesn't sufficiently narrow the type. There is a case where sourceData is CollectionData but the length of the images is 0, so when setSourceInfo is called the type of sourceData: SourceData | CollectionData. If were just,

   if ('images' in sourceData) {
      // This is a Collection

Typescript could appropriately narrow correctly infer sourceData: SourceData

@manzt
Copy link
Member

manzt commented May 27, 2022

@will-moore, The ImageLayer from Viv has an updated API since they last time you worked on this (which was upgrade in #126). I fixed (from what I can tell) the rendering, and you will just need to figure out how to resolve the one type error from above.

@manzt
Copy link
Member

manzt commented May 27, 2022

image

@will-moore
Copy link
Collaborator Author

Thanks @manzt - I'm away for rest of this week but will take a look ASAP...

@will-moore
Copy link
Collaborator Author

Hi @manzt - Unfortunately I have no clue how to fix:

src/vizarr.tsx:29:19 - error TS2345: Argument of type '(prevSourceInfo: WithId<SourceData>[]) => (WithId<SourceData> | { name?: string | undefined; images: any[]; id: string; })[]' is not assignable to parameter of type 'SetStateAction<WithId<SourceData>[]>'.
  Type '(prevSourceInfo: WithId<SourceData>[]) => (WithId<SourceData> | { name?: string | undefined; images: any[]; id: string; })[]' is not assignable to type '(prev: WithId<SourceData>[]) => WithId<SourceData>[]'.
    Type '(WithId<SourceData> | { name?: string | undefined; images: any[]; id: string; })[]' is not assignable to type 'WithId<SourceData>[]'.
      Type 'WithId<SourceData> | { name?: string | undefined; images: any[]; id: string; }' is not assignable to type 'WithId<SourceData>'.
        Type '{ name?: string | undefined; images: any[]; id: string; }' is not assignable to type 'WithId<SourceData>'.
          Type '{ name?: string | undefined; images: any[]; id: string; }' is missing the following properties from type 'SourceData': loader, channel_axis, colors, names, and 5 more.

29     setSourceInfo((prevSourceInfo) => {

That line didn't change in this PR, so I'm not sure where to start...?

@manzt
Copy link
Member

manzt commented Jun 7, 2022

That line didn't change in this PR, so I'm not sure where to start...?

Hi will, the issue is that this PR widens the return type of createSourceData:

// previously SourceData, now SourceData | CollectionData
const sourceData = await createSourceData(config);

TypeScript is forcing you to handle each case (SourceData and CollectionData) in the rest of the function body, and the compiler error is saying the current code does not sufficiently handle the types as you intended and there is a bug. To demonstrate,

const sourceData: SourceData | CollectionData = { images: [], /* ... */ }; // we know this is a collection, but TS does not yet
if ('images' in sourceData && images.length > 0) {
  sourceData // CollectionData, we know this can only be CollectionData
  setCollectionState(sourceData);
  return;
};
sourceData // SourceData | CollectionData! source data _or_ collection with empty image list

The issue is that the guard 'images' in sourceData && images.length > 0 doesn't capture all instances of CollectionData since the expression isn't always true for CollectionData (e.g., a collection with images.length === 0), so the type is not sufficiently narrowed to just SourceData after the if block with early return.

setSourceInfo can only handle SourceData in the return so TS is letting you know there is a case where CollectionData can reach this bit of code and that is invalid. You can easily remove errors but forcing this condition to be more strict:

const sourceData: SourceData | CollectionData = { images: [], /* ... */ }
if ('images' in sourceData) {
  sourceData // CollectionData
  setCollectionState(sourceData);
  return;
};
sourceData // SourceData since the above captures _all_ instances of CollectionData

@will-moore
Copy link
Collaborator Author

Got it! Thanks for the explanation @manzt

@will-moore
Copy link
Collaborator Author

See #149 (comment)

@will-moore will-moore closed this Jul 6, 2022
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.

2 participants