Load single slices at higher resolution #163

frasercl · 2023-11-06T21:32:22Z

Resolves #157: If a 3d volume is loaded, the user switches to z-slice mode, and a higher resolution level is available to load, reload at that higher resolution.

This is accomplished using the loadDims method on IVolumeLoader (unused until now), which loads the dimensions of a volume without loading any data. I made the following changes to make loadDims a good solution to this issue:

loadDims returns an array of VolumeDims objects for each available scale level. I added the property canLoad: boolean to this type to indicate whether a level can be loaded given the specifications.
Because loadDims is async, updateRequiredData must now be async too. This is a bummer, so to compensate, I tried to ensure that loadDims rarely or never is async in practice.

I also got rid of the scene property of LoadSpec, given that the only loader that supports scenes right now (OmeZarrLoader) must be bound to a single scene on construction.

src/Volume.ts

toloudis · 2023-11-07T17:17:24Z

src/loaders/IVolumeLoader.ts

  // shape: [t, c, z, y, x]
  shape: number[] = [0, 0, 0, 0, 0];
  // spacing: [t, c, z, y, x]; generally expect 1 for non-spatial dimensions
  spacing: number[] = [1, 1, 1, 1, 1];
  spaceUnit = "μm";
  timeUnit = "s";
+  canLoad = true;


I suggest an explanatory comment here, something like:

// dims are small enough to fit within our preferred memory footprint (see `pickLevelToLoad`)

wondering if this class is even the right place to store this...

toloudis · 2023-11-07T17:22:20Z

src/Volume.ts

+      const minScale = this.loadSpec.multiscaleLevel ?? 0;
+      // Loaders should cache loaded dimensions so that this call blocks no more than once per valid `LoadSpec`.
+      const dims = await this.loader?.loadDims(this.loadSpecRequired);
+      if (dims && currentScale > minScale && dims[currentScale - 1].canLoad) {


I wonder if the canLoad could be a dynamic check here? I don't think it's too slow to call a function like pickLevelToLoad if it doesn't require network fetches but I forget how frequently this gets called.

I guess I felt like the decision of which level to load was the loader's responsibility, since pickLevelToLoad and the MAX_ATLAS_DIMENSION constant live in the same file as OMEZarrLoader. I kind of agree though that this ought to be treated more like a global limit. Do we make that official by moving those items into VolumeLoaderUtils and importing/calling pickLevelToLoad here? This is as opposed to calling it internally in loadDims, which is what is done now.

pickLevelToLoad itself doesn't require network fetches, it just takes dimensions and performs a bit of logic. Those dimensions, however, need to be fetched. This is the problem I referred to in the PR description and in the comment on line 251 above: loaders need to be smart enough to ensure that providing dimensions rarely requires a fetch in practice. The behavior in this PR is:

In OMEZarrLoader, all dims are fetched before loader construction and loadDims will never block

In TiffLoader, metadata is fetched on the first call to loadDims or loadVolumeData and cached (so if the volume already has data, loadVolumeData has been called and loadDims won't block in practice)

In JsonImageInfoLoader, metadata is fetched on the first call to loadDims or loadVolumeData per timestep and cached. In practice, if this function calls loadVolumeData after calling loadDims, the first loadDims call will have cached the metadata that the loadVolumeData would have required so checking dims first adds no additional work.

Moving the pickLevelToLoad call won't change these rules on when this function has to wait for the network.

Yea I basically agree with everything you said here.

pickleveltoload can be a utils-like shared function, or even a IVolumeLoader method that returns 0 by default and only does something nontrivial in omezarrloader.

all dims could be fetched at or close to loader creation time. Then they will just be available and no call to loadDims would have to fetch anything (though they could if they wanted?)

Co-authored-by: toloudis <toloudis@users.noreply.github.com>

ShrimpCryptid · 2023-11-07T20:14:45Z

src/loaders/JsonImageInfoLoader.ts

+  private async getJsonImageInfo(time: number): Promise<JsonImageInfo> {
+    const cachedInfo = this.jsonInfo[time];
+    if (cachedInfo) {
+      return cachedInfo;
+    }
+
+    const response = await fetch(this.urls[time]);


I like this optimization!

ShrimpCryptid

Agree with Dan that I'm not totally sold on VolumeDims tracking whether it can be loaded? It seems like that should be tracked by a system external to it, but I'm fine to talk about tradeoffs too.

…animated/volume-viewer into fix/load-hires-slice

src/Volume.ts

ShrimpCryptid · 2023-11-09T23:51:02Z

I tried running this to test it, and I ran into an error?

Repro:

Run the project (npm run start)
Switch to OME Zarr (NucMorph)
Switch to Z viewer

Uncaught Runtime Errors:
ERROR
chunk request cancelled
    at handleError (http://localhost:9020/volume-viewer-ui.bundle.js:25109:58)
    at http://localhost:9020/volume-viewer-ui.bundle.js:25132:7
ERROR
chunk request cancelled
    at handleError (http://localhost:9020/volume-viewer-ui.bundle.js:25109:58)
    at http://localhost:9020/volume-viewer-ui.bundle.js:25132:7
ERROR
chunk request cancelled
    at handleError (http://localhost:9020/volume-viewer-ui.bundle.js:25109:58)
    at http://localhost:9020/volume-viewer-ui.bundle.js:25132:7

It's possible this is related to the Vast outage today. Will check again later.

ShrimpCryptid · 2023-11-10T16:52:17Z

I tried running this to test it, and I ran into an error?

Repro:

Run the project (npm run start)
Switch to OME Zarr (NucMorph)
Switch to Z viewer

Uncaught Runtime Errors:
ERROR
chunk request cancelled
    at handleError (http://localhost:9020/volume-viewer-ui.bundle.js:25109:58)
    at http://localhost:9020/volume-viewer-ui.bundle.js:25132:7
ERROR
chunk request cancelled
    at handleError (http://localhost:9020/volume-viewer-ui.bundle.js:25109:58)
    at http://localhost:9020/volume-viewer-ui.bundle.js:25132:7
ERROR
chunk request cancelled
    at handleError (http://localhost:9020/volume-viewer-ui.bundle.js:25109:58)
    at http://localhost:9020/volume-viewer-ui.bundle.js:25132:7

It's possible this is related to the Vast outage today. Will check again later.

Still getting this bug today.

This is all I'm getting from the console:

index.ts:872 currentVol with name AICS-12_881 is loaded
index.ts:872 currentVol with name P13-C4 is loaded
localhost/:1 Uncaught (in promise) chunk request cancelled
index.ts:872 currentVol with name P13-C4 is loaded

toloudis · 2023-11-10T17:55:52Z

If I have any coding time today I'll try to take a look at this error message. This is an important change to get in soon.

toloudis · 2023-11-13T21:29:41Z

The error here is indeed an uncaught promise exception that occurs when we cancel a request.
This pull request : gzuidhof/zarr.js#145 actually seems to fix the issue. (also see gzuidhof/zarr.js#140) The problem seems to be that our promises get stuck in the zarr.js plumbing because of their internal logic in between our proxy store's SmartStoreWrapper.getRaw and the resulting multiple calls to the true store's getItem.

You can see some hints of this by changing
return this.requestQueue.addRequest(key, () => this.getAndCacheItem(item, key, opts));
to

return this.requestQueue
        .addRequest(key, () => this.getAndCacheItem(item, key, opts))
        .catch((e) => {
          if (e !== CHUNK_REQUEST_CANCEL_REASON) {
            throw e; // this will be unhandled!!!
          }
          // if cancelled, we still need to return an arraybuffer of the expected size,
          // or else handle this one - returning this buffer still breaks things.
          return new ArrayBuffer(0);
        });

Another way of saying this is:
We call getRaw to fetch some data. The getRaw call is converted into many getItem calls by zarr.js's internals. We are overriding getItem in our SmartStoreWrapper. Each getItem becomes a cancellable request promise generated by our RequestQueue. These getItem requests are per-chunk, and are not propagated back out of the SmartStoreWrapper when getRaw finishes.

Co-authored-by: Peyton Lee <peyton.lee@alleninstitute.org>

frasercl added 12 commits November 3, 2023 17:38

update VolumeDims; update loaders to match

f3b17ae

make JsonImageInfoLoader smarter about caching metadata

2ca82c4

fixing and use loadDims to trigger reloads (first try)

99fd627

tiff loader caches dims

9cc5937

remove now-redundant properties from TiffLoader

f8dade5

simplify tiff dimension loading

bf76807

add multiscale level to ImageInfo

a3de8e3

make loading higher resolutions work, for real

eac25c9

clean up logic around when to load new data

31baa7f

emphasize that zarr loader will never block when loading dims

b6f5ce3

remove scene from LoadSpec

574b238

fix ImageInfo type errors

52d2bdd

frasercl requested a review from a team as a code owner November 6, 2023 21:32

frasercl requested review from toloudis, meganrm, blairlyons, interim17, ShrimpCryptid and ascibisz and removed request for a team November 6, 2023 21:32

frasercl changed the title ~~Fix/load hires slice~~ Load single slices at higher resolution Nov 6, 2023

toloudis reviewed Nov 7, 2023

View reviewed changes

src/Volume.ts Outdated Show resolved Hide resolved

toloudis reviewed Nov 7, 2023

View reviewed changes

fix off-by-one in ImageInfo doc comment

736640d

Co-authored-by: toloudis <toloudis@users.noreply.github.com>

ShrimpCryptid reviewed Nov 7, 2023

View reviewed changes

frasercl added 3 commits November 8, 2023 10:21

remove an unnecessary branch

2a45383

move pickLevelToLoad out of OmeZarrLoader

d862c4f

add a bit more documentation to VolumeLoaderUtils

61a76d4

frasercl added 6 commits November 8, 2023 14:59

turns out pickLevelToLoad is pretty zarr-specific... put it back

735d5b8

Volume calls estimateLevelForAtlas on its own

0145f2d

remove canLoad property from VolumeDims

dd48d7e

fix lint: don't destructure unused fields from axesTCZYX

6ea4b23

remove unused import

27d8705

Merge branch 'fix/load-hires-slice' of https://github.com/allen-cell-…

a8f254e

…animated/volume-viewer into fix/load-hires-slice

ShrimpCryptid reviewed Nov 9, 2023

View reviewed changes

src/Volume.ts Outdated Show resolved Hide resolved

This was referenced Nov 15, 2023

better single slice render #165

Merged

Fix error when cancelling zarr requests #167

Closed

remove redundant null check

979c3f1

Co-authored-by: Peyton Lee <peyton.lee@alleninstitute.org>

toloudis approved these changes Nov 27, 2023

View reviewed changes

toloudis merged commit 2279053 into main Nov 27, 2023
3 checks passed

toloudis deleted the fix/load-hires-slice branch November 27, 2023 19:00

frasercl mentioned this pull request Apr 12, 2024

multiresolution in 2d mode #91

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load single slices at higher resolution #163

Load single slices at higher resolution #163

frasercl commented Nov 6, 2023

toloudis Nov 7, 2023

toloudis Nov 7, 2023

toloudis Nov 7, 2023 •

edited

Loading

frasercl Nov 7, 2023

toloudis Nov 7, 2023

ShrimpCryptid Nov 7, 2023

ShrimpCryptid left a comment

ShrimpCryptid commented Nov 9, 2023

ShrimpCryptid commented Nov 10, 2023 •

edited

Loading

toloudis commented Nov 10, 2023 •

edited

Loading

toloudis commented Nov 13, 2023 •

edited

Loading

Load single slices at higher resolution #163

Load single slices at higher resolution #163

Conversation

frasercl commented Nov 6, 2023

toloudis Nov 7, 2023

Choose a reason for hiding this comment

toloudis Nov 7, 2023

Choose a reason for hiding this comment

toloudis Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

frasercl Nov 7, 2023

Choose a reason for hiding this comment

toloudis Nov 7, 2023

Choose a reason for hiding this comment

ShrimpCryptid Nov 7, 2023

Choose a reason for hiding this comment

ShrimpCryptid left a comment

Choose a reason for hiding this comment

ShrimpCryptid commented Nov 9, 2023

ShrimpCryptid commented Nov 10, 2023 • edited Loading

toloudis commented Nov 10, 2023 • edited Loading

toloudis commented Nov 13, 2023 • edited Loading

toloudis Nov 7, 2023 •

edited

Loading

ShrimpCryptid commented Nov 10, 2023 •

edited

Loading

toloudis commented Nov 10, 2023 •

edited

Loading

toloudis commented Nov 13, 2023 •

edited

Loading