Communicate truncated 64 bit segmentation #4598

youri-k · 2020-05-05T16:38:16Z

URL of deployed dev instance (used for testing):

https://communicatetruncateduint.webknossos.xyz

Steps to test:

abc

Issues:

fixes Communicate to the user if 64 bit segmentation ids are truncated to 32 bit #4492

Updated (unreleased) changelog
~~[ ] Updated (unreleased) migration guide if applicable~~
~~[ ] Updated documentation if applicable~~
~~[ ] Adapted wk-connect if datastore API changes~~
~~[ ] Needs datastore update after deployment~~
Ready for review

* not working as intended as the segmentation is 32 bit for the frontend

…egmentation layer

fm3

Backend LGTM and works for me :)

…n-for-id-mismatch-in-64-bit-segmemtations

…unicate-truncated-uint64

…int64

MichaelBuessemeyer · 2020-05-07T13:38:40Z

@daniel-wer Would you please check the frontend part?

youri-k · 2020-05-07T14:02:49Z

frontend/javascripts/oxalis/model_initialization.js

@@ -433,10 +433,14 @@ function setupLayerForVolumeTracing(
  const fallbackLayerIndex = _.findIndex(layers, layer => layer.name === tracing.fallbackLayer);
  const fallbackLayer = layers[fallbackLayerIndex];
  const boundaries = getBoundaries(dataset);
+  // The frontend does not support uint64 data. Thus if the segmentation is uint64
+  // we treat it as uint32 and save that it originally has the element class uint64.
+  const isUint64Segmentation = tracing.elementClass === "uint64";


What is tracing.elementClass referring to, the elementClass or the originalElementClass?
The backend converts an elementClass of uint64 to uint32, so the elementClass value should never be uint64.
This is how we achieved the original cheap uint64 support, see #4233. If you want to, the backend can send the real elementClass to the frontend, which would mean we could rid of the originalElementClass in the backend.

this is in case of volume tracings (as opposed to view mode). the tracing still sends the uint64.
but yeah, feel free to unify this in whichever way seems best :)

I think we need to be extra careful before allowing 64 bit volume tracings, as they will probably mix 32 bit and 64 bit data. But this PR mainly deals with the view mode, right @MichaelBuessemeyer ?

I got no mail notification that youri posted this concern 😕.

But this PR mainly deals with the view mode, right @MichaelBuessemeyer ?

No it doesn't This PR also enables 64-bit volume tracings in the frontend. If we do not what this behaviour (and that's what I think is your opinion) we just have to remove the changes in lines 436-438, 442. Then the user will get a warning upon opening a volume tracing.

@youri: When the user creates a volume tracing the backend creates an empty tracing (maybe including a fallback layer). At this point the backend seems inconsistent, because trying to create a volume tracing on a dataset with a uint64-bit segmentation layer will set the elementClass of the initial tracing to elementClass. At least that's what I remember and why I needed to add the lines 436-438, 442.

According to the suggestion to always send the 64-bit element class as the property elementClass and getting rid of originalElementClass: This should be possible :).

I see, thank you for your reply!

We should extensively test the 64 bit volume tracing, especially with how fallback layer data is incorporated into the tracing, and if this works with #4602
But yeah, let’s try it :)

I just had a conversation with Youri and we decided it would be preferrable not to allow 64-bit volume tracings yet.

While the trouble with their data being in 32 bit and the fallback buckets being 64 bit is invisible in the wK frontend, since the fallback buckets will be truncated to 32 bit, the data will not actually be compatible, it will not actually be clear which ID should belong to which cell.

Additionally, saving 32 bit data labeled as 64 bit data can lead to incompatibilities in case the frontend in some future version using some future javascript features actually produces 64 bit data. Then our implicit knowledge becomes ambiguous.

Since no user has yet wished for volume tracings with 64 bit fallback layer, and this PR was originally only about the user notification about truncated data, please remove the changes you mentioned, @MichaelBuessemeyer , so that the user can not trace on 64 bit volume tracings. Additionally, @youri-k , please add an check in the back-end side creation of volume tracings, so that 64 bit volume tracings can not be created.

…rontend

…municate-truncated-uint64

MichaelBuessemeyer · 2020-05-14T13:41:57Z

@daniel-wer Could you please review the frontend part of this PR?

I added the optional originalElementClass property to the segmentation layer type. This makes it possible to know that the original element class on the server is unit64 and not the truncated uint32 type.

Just saving the uint64 as the elementClass might make things a bit more complicated because we have to touch all the code that relies on the fact that the element class of the segmentation can never be uint64.

The conversion of type uint64 to uint32 is not done for the segmentation layer if we have a volume tracing as volume tracings for uint64 segmentation data are currently not allowed.

daniel-wer

@MichaelBuessemeyer Frontend looks good to me, I added a couple of suggestions. I couldn't test yet, but will report back after the CI run was successful :)

frontend/javascripts/messages.js

daniel-wer · 2020-05-19T09:37:07Z

frontend/javascripts/oxalis/model_initialization.js

@@ -295,6 +295,24 @@ function initializeDataset(
    dataSet: dataset.dataSource.id.name,
  });

+  // Here we add the originalElementClass property to the segmentation layer if it exists.


Might be a matter of personal preference, but I think usually comments should short and concise, so Add the original ... instead of Here we add the original .... Please drop the we from the other comments as well. Apart from that, the comments are written very well 👍

Ok, thanks for that feedback. I'll pay attention to that 🙂

daniel-wer · 2020-05-19T09:37:36Z

frontend/javascripts/oxalis/model_initialization.js

@@ -434,9 +452,13 @@ function setupLayerForVolumeTracing(
  const fallbackLayer = layers[fallbackLayerIndex];
  const boundaries = getBoundaries(dataset);

+  // We do not adjust the elementClass here if it is uint64, because uint64 volume tracings are not supported.
+  // Not adjusting the elementClass will cause the later check for supported layer element classes to fail
+  // if the segmentation of type uint64.


Suggested change

// if the segmentation of type uint64.

// if the segmentation is of type uint64.

daniel-wer · 2020-05-19T09:40:01Z

frontend/javascripts/oxalis/model_initialization.js

  const tracingLayer = {
    name: tracing.id,
    elementClass: tracing.elementClass,
+    originalElementClass: tracing.elementClass,


Does the originalElementClass need to be set here at all? In the type definition it's declared as optional.

Thanks, you are correct. I'll revert the changes here 👍

daniel-wer · 2020-05-19T09:52:37Z

frontend/javascripts/oxalis/view/right-menu/mapping_info_view.js

+        <React.Fragment>
+          {title}{" "}
+          <Tooltip title={message["tracing.uint64_segmentation_warning"]}>
+            <Icon type="info-circle" style={{ color: "rgb(255, 155, 85)" }} />


Maybe use warning as the type instead. That would better fit with the color and the intention of this message to warn the user that the displayed ids may not be correct.

Sure, that makes sense 🤯

daniel-wer · 2020-05-19T10:38:30Z

Testing went well 👍

@youri-k How much effort would it be to make this error message a little more helpful and display the actual error message concerning the uint64 layer instead of the generic one in the top level? It's fine if it's not feasible, I just wanted to check :)

fm3 · 2020-05-19T10:43:29Z

I think it should be relatively easy to move the “because you are not a memeber of a team that has access” down into the debug info chain (so that it only shows when it is actually true).
This way the top-level error stays the generic »failed to create annotation« but upon expanding the correct reason is available.
We could even consider renaming the expand button to »show more information« rather than just »debug« because I think there are a number of occasions where this info is also helpful to the user (or at least their team manager).
What do you think?

youri-k · 2020-05-19T10:59:12Z

I'll do the suggested changes and I'm also in favor of renaming the expand button.

…municate-truncated-uint64

Co-authored-by: Daniel <[email protected]>

MichaelBuessemeyer · 2020-05-19T13:19:19Z

@daniel-wer Could you please check the few changes I pushed?

daniel-wer

@MichaelBuessemeyer Frontend, LGTM 👍

youri-k · 2020-05-20T12:33:05Z

This should be good to go, or @MichaelBuessemeyer?

MichaelBuessemeyer · 2020-05-20T12:42:07Z

If the backend part is also approved then sure 🎉

MichaelBuessemeyer · 2020-05-20T13:18:18Z

@youri-k Is the backend part also approved?

MichaelBuessemeyer and others added 2 commits April 23, 2020 21:53

add warning to user if 64-segmentation is active

d87e6bf

* not working as intended as the segmentation is 32 bit for the frontend

add optional originalElementClass field, which is filled for uint64 s…

dfbeeb3

…egmentation layer

youri-k added backend frontend labels May 5, 2020

youri-k requested a review from fm3 May 5, 2020 16:38

youri-k assigned youri-k and MichaelBuessemeyer May 5, 2020

fm3 reviewed May 6, 2020

View reviewed changes

MichaelBuessemeyer added 4 commits May 7, 2020 12:47

Merge branch 'master' of github.com:scalableminds/webknossos into war…

3b29a18

…n-for-id-mismatch-in-64-bit-segmemtations

Merge branch 'warn-for-id-mismatch-in-64-bit-segmemtations' into comm…

770befa

…unicate-truncated-uint64

use original element class to determine whether the segmentation is u…

de69d27

…int64

add changelog entry

0c5ac03

MichaelBuessemeyer requested a review from daniel-wer May 7, 2020 13:38

MichaelBuessemeyer marked this pull request as ready for review May 7, 2020 13:38

youri-k commented May 7, 2020

View reviewed changes

Youri K and others added 7 commits May 13, 2020 11:27

remove original element class and always send real element class to f…

4663bf8

…rontend

merge master

ba6a2cf

fix typo

5a1dc75

Merge branch 'master' of github.com:scalableminds/webknossos into com…

aedfc6e

…municate-truncated-uint64

add check so volumes can not be created for uint64 fallback layer

4b58075

added manually saving of the originalElementClass in the frontend

0116428

Merge branch 'master' of github.com:scalableminds/webknossos into com…

c892cbf

…municate-truncated-uint64

daniel-wer reviewed May 19, 2020

View reviewed changes

Youri K added 2 commits May 19, 2020 14:21

Merge branch 'master' of github.com:scalableminds/webknossos into com…

04bd5ce

…municate-truncated-uint64

update error message

26086ec

MichaelBuessemeyer and others added 2 commits May 19, 2020 14:58

Update frontend/javascripts/messages.js

3372fbd

Co-authored-by: Daniel <[email protected]>

make comments more concise and remove unnecessary changes

9b6594d

daniel-wer approved these changes May 19, 2020

View reviewed changes

Merge branch 'master' into communicate-truncated-uint64

3951b3e

daniel-wer and others added 2 commits May 20, 2020 15:25

Change "Show debug information" in toast to "Show more information"

94aec77

Merge branch 'master' into communicate-truncated-uint64

8910068

youri-k merged commit 9bb14a6 into master May 27, 2020

philippotto deleted the communicate-truncated-uint64 branch June 14, 2022 11:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Communicate truncated 64 bit segmentation #4598

Communicate truncated 64 bit segmentation #4598

youri-k commented May 5, 2020 •

edited

Loading

fm3 left a comment

MichaelBuessemeyer commented May 7, 2020

youri-k May 7, 2020 •

edited

Loading

fm3 May 7, 2020

fm3 May 11, 2020 •

edited

Loading

MichaelBuessemeyer May 11, 2020

fm3 May 11, 2020

fm3 May 13, 2020

MichaelBuessemeyer commented May 14, 2020 •

edited

Loading

daniel-wer left a comment

daniel-wer May 19, 2020

MichaelBuessemeyer May 19, 2020

daniel-wer May 19, 2020

daniel-wer May 19, 2020

MichaelBuessemeyer May 19, 2020

daniel-wer May 19, 2020

MichaelBuessemeyer May 19, 2020

daniel-wer commented May 19, 2020

fm3 commented May 19, 2020 •

edited

Loading

youri-k commented May 19, 2020 •

edited

Loading

MichaelBuessemeyer commented May 19, 2020

daniel-wer left a comment

youri-k commented May 20, 2020

MichaelBuessemeyer commented May 20, 2020

MichaelBuessemeyer commented May 20, 2020

	// if the segmentation of type uint64.
	// if the segmentation is of type uint64.

Communicate truncated 64 bit segmentation #4598

Communicate truncated 64 bit segmentation #4598

Conversation

youri-k commented May 5, 2020 • edited Loading

URL of deployed dev instance (used for testing):

Steps to test:

Issues:

fm3 left a comment

Choose a reason for hiding this comment

MichaelBuessemeyer commented May 7, 2020

youri-k May 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fm3 May 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelBuessemeyer commented May 14, 2020 • edited Loading

daniel-wer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-wer commented May 19, 2020

fm3 commented May 19, 2020 • edited Loading

youri-k commented May 19, 2020 • edited Loading

MichaelBuessemeyer commented May 19, 2020

daniel-wer left a comment

Choose a reason for hiding this comment

youri-k commented May 20, 2020

MichaelBuessemeyer commented May 20, 2020

MichaelBuessemeyer commented May 20, 2020

youri-k commented May 5, 2020 •

edited

Loading

youri-k May 7, 2020 •

edited

Loading

fm3 May 11, 2020 •

edited

Loading

MichaelBuessemeyer commented May 14, 2020 •

edited

Loading

fm3 commented May 19, 2020 •

edited

Loading

youri-k commented May 19, 2020 •

edited

Loading