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

Fix rendering missing data black and wrong segment id being displayed #5674

Merged
merged 8 commits into from
Aug 23, 2021

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Aug 13, 2021

This PR tackles two issues:

  1. When "Render missing data black" was turned off and the data of the fallback mag for the segmentation layer was visible, all segments had id 0. This was due to not looking up the segment id in the fallback bucket. The data was not looked up, because all buckets from a non-existing magnification for the data layer currently in question (e.g. segmentation layer) were null buckets. The previous code assumed, that if we have null buckets anyway, looking up in other mags does not benefit at all. But this is wrong in case the above-mentioned option is turned off. Thus this pr simply changes this assumption, if the "Render missing data black"-option is turned off.
  2. When the "Render missing data black"-option was turned on and a layer does not have all mags, the existing mags were only rendered as fallback data if the current mag was only one zoomStep lower. Although it should be expected that the fallback data should be rendered up to the mag that is three steps lower. This PR removes this restriction. The restriction was previously added, because, with each step, the fallback and the rendered mag are apart, the number of entries in the lookup table grows exponentially, as each bucket in the actually rendered mag needs an entry added to the lookup table. But this overall does not matter as when there was actually data available for the currently rendered magnification, each bucket in the current mag would also need an entry in the lookup table. Thus in both cases the number of entries in the lookup table does not change.
    In the first case, there are simply much fewer buckets of the fallback mag (exponentially decreasing with the mag difference) that are added for many buckets in the rendered mag (exponentially increasing with the mag difference) to the lookup table. Thus the exponential increase and decrease should cancel out each other.

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

Better test locally. First, read the whole steps. It would be good if you have another dataset that has a segmentation for all 16 mags and a color layer with all 16 mags. This might show some edge cases that did not appear with the ROI dataset.

  • First set up a new ROI Dataset for testing. For that dataset create a copy of the color layer and only keep mag 16. Then also delete all mags of the segmentation layer except for mag 16.
  • Go to the dataset list, open the settings of the ROI dataset and save the suggested setting.
  • View the dataset and turn off the original color layer and the segmentation layer in the layer settings. Then turn off the "Render missing data black" setting.
  • You shouldn't be able to see data in mag 1, 32. But you should be able to see data in the other mags.
  • The same should be the case for the manipulated color layer. To test this, turn on the color layer and turn of the segmentation layer.
  • Only make the unchanged color layer visible. Here you should see data in each resolution.
  • Now turn on the unchanged color layer and the segmentation layer. Check whether you get a segmentation id for any hovered segment in each mag, in which the segmentation is visible. If the magnifications where the segmentation is visible, the displayed segment id in the status bar should be 0.

Issues:


@MichaelBuessemeyer MichaelBuessemeyer self-assigned this Aug 13, 2021
@MichaelBuessemeyer MichaelBuessemeyer changed the title Fix rendering missing data black Fix rendering missing data black and wrong segment id being displayed Aug 13, 2021
Comment on lines -611 to -628
onChangeRenderMissingDataBlack = async (value: boolean): Promise<void> => {
Toast.info(
value
? messages["data.enabled_render_missing_data_black"]
: messages["data.disabled_render_missing_data_black"],
{ timeout: 8000 },
);
this.props.onChange("renderMissingDataBlack", value);
const { layers } = this.props.datasetConfiguration;
const reloadAllLayersPromises = Object.keys(layers).map(async layerName => {
await clearCache(this.props.dataset, layerName);
await api.data.reloadBuckets(layerName);
});
await Promise.all(reloadAllLayersPromises);
window.needsRerender = true;
Toast.success("Successfully reloaded data of all layers.");
};

Copy link
Contributor Author

@MichaelBuessemeyer MichaelBuessemeyer Aug 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method was used nowhere. It was forgotten to be removed while reorganizing code in the #5384 pr

@MichaelBuessemeyer
Copy link
Contributor Author

The performance test:

How I did it:
I simply wrapped the whole body of the _refreshLookUpBuffer method with a console.time() and a console.timeEnd().
I used the ROI dataset prepared as described above with only the segmentation layer on in mag 2. I moved around a bit and took the avg and variance of the measured times.
Then I did the same on an unchanged ROI dataset with only the segmentation layer enabled in mag 2.

This is the result of the performance test on this branch with only the segmentation layer enabled in mag2 using the mag 16 as a fallback:
image

This is the result of the performance test on master with only the segmentation layer enabled in mag 2:
image

Tool used for these calculations: https://www.calculatorsoup.com/calculators/statistics/statistics.php

Thus rendering the filling the lookup buffer with fallback bucket addresses should be even faster than filling it with data from the current mag and should not make any performance difficulties.

Copy link
Member

@philippotto philippotto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really great PR 👍 I especially like your detailed description of status quo, old reasonings and the solution.

Regarding your performance measurements: Am I getting this right, that the look up buffer is quicker to update when it's only filled with fallback data? If so: perfect 😄

@MichaelBuessemeyer
Copy link
Contributor Author

Am I getting this right, that the look up buffer is quicker to update when it's only filled with fallback data?

Yeah, I came to the same conclusion 🤷‍♂️ 🎉. Maybe it is faster because fewer buckets need to be looked up 🤷‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mag rendering and Warning icon are out of sync (if “render missing data black” deactivated)
2 participants