magland / figurl-franklab-views

0 stars 0 forks source link

'impossible situation' error in DecodedLinearPositionDrawing.ts #16

Closed magland closed 1 year ago

magland commented 1 year ago

In DecodedLinearPositionDrawing.ts there was an error being thrown

throw Error(`Impossible situation: requested window ${downsampledRangeStart}-${downsampledRangeEnd} does not fit in canvas width ${canvas.width} as allowed by current scale factor ${scale}`)

which was causing the program to crash. I changed this to a warning for now. If you look at the below example, you'll see that warning in the console as you zoom/in out, for certain window sizes. I wasn't able to track down why that situation should be impossible. I was also noticing some other weird behavior as I zoomed in/out.

https://figurl.org/f?v=gs://figurl/spikesortingview-10&d=sha1://263ce2572debd5ed3b8fdceef922bad72e5644e6&label=tonks20211025_.nwb_run2_reward_mua_filter&zone=default

jsoules commented 1 year ago

This error occurs if (downsampledRangeEnd - downsampledRangeStart) > canvas.width, i.e. after downsampling we have more data points than pixels available to display them in.

This should not happen, because the downsampling logic is looking for a downsampling factor that makes it not true. See DecodedLinearPositionDownsampling.ts, computeScaleFactor:

export const computeScaleFactor = (baseScaleFactor: number, visibleRangeCount: number, maxRangeCount: number) => {
    // We want x, the smallest power of base s.t. (the visible range) * base^x > full range.
    // So if we have 2000 columns visible and can display up to 750, with base scaling of 3, we would need
    // to compress 27x (3^3) so that 27 native columns make up every display column.
    const exponent = Math.ceil(Math.log(visibleRangeCount/maxRangeCount)/Math.log(baseScaleFactor))
    return Math.max(Math.pow(baseScaleFactor, exponent), 1)
}

I think the issue is that where computeScaleFactor gets called in DecodedLinearPositionPlotView the line is

const scaleFactor = computeScaleFactor(BASE_SCALE_FACTOR, visibleFrameRange, MAX_WIDTH_FOR_SCALING)

where BASE_SCALE_FACTOR is some predetermined value (we currently use 3), visibleFrameRange is the number of data frames in the viewport, and MAX_WIDTH_FOR_SCALING is a constant currently set to 2000.

I think what's going on is that reference to MAX_WIDTH_FOR_SCALING is incorrect--it should be the lesser of the hard maximum, and the actual width of the canvas. Otherwise we'll come up with a scale factor that works fine for a 2000-pixel-wide canvas, but doesn't compress enough for a canvas that's much smaller than that.

In other words, this would be expected to fail for narrow window widths.

Let me see if I can reproduce the error.

jsoules commented 1 year ago

Okay, looks like I can reproduce with the above link if I set the window size such that the canvases are substantially smaller than 2000 pixels wide.

magland commented 1 year ago

I think I was able to reproduce even with larger width. Let me try again.

jsoules commented 1 year ago

Ahhh, okay. This is a different thing, I was wrong about the above.

In context, the code is working on the offscreen canvas, which isn't limited by the size of the visible canvas--it really is just a hyperparameter.

The tricky thing here is that we are right-sizing the offscreen canvas so that it is not wider than the width of the entire data set after downsampling--see DecodedLinearPositionPlotView:

const canvasTargetWidth = useMemo(() => Math.min(MAX_OFFSCREEN_CANVAS_WIDTH, sampledData.downsampledTimes.length), [sampledData.downsampledTimes.length])

This makes sense, since there's no reason to spend more memory on the offscreen buffer than there is data available to fill it.

The trick here is that the width of the offscreen canvas is set according to the size of the downsampled data, i.e. the sampledData.downsampledTimes.length. For this data set that is 1087 data points.

However, the check for whether the data fits is based on the visible time start and end; see DecodedLinearPositionDrawing:

    if ((downsampledRangeEnd - downsampledRangeStart) > (canvas.width)) {
        console.warn(`Impossible situation: requested window ${downsampledRangeStart}-${downsampledRangeEnd} does not fit in canvas width ${canvas.width} as allowed by current scale factor ${scale}`)
    }

What's happened here is that this data set has time points without corresponding observations (whether through discontinuity or ending early). The time codes go from 0 - 1251, but the length of the downsampled data set is only 1087. So we size the canvas to fit the 1087 available data points, but then it looks like an error when the desired space to write them in exceeds that value.

magland commented 1 year ago

I was able to reproduce for a canvas width > 2000.

This was when the zoom level went beyond the bounds of the decode data (the timeseries graph data seems to be longer in duration). And something funky is happening to the position plot in this case as well.

image

jsoules commented 1 year ago

Resolved by PR #17.

magland commented 1 year ago

This fix has now been deployed.