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

Communicate truncated 64 bit segmentation #4598

Merged
merged 20 commits into from
May 27, 2020
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
d87e6bf
add warning to user if 64-segmentation is active
MichaelBuessemeyer Apr 23, 2020
dfbeeb3
add optional originalElementClass field, which is filled for uint64 s…
May 5, 2020
3b29a18
Merge branch 'master' of github.com:scalableminds/webknossos into war…
MichaelBuessemeyer May 7, 2020
770befa
Merge branch 'warn-for-id-mismatch-in-64-bit-segmemtations' into comm…
MichaelBuessemeyer May 7, 2020
de69d27
use original element class to determine whether the segmentation is u…
MichaelBuessemeyer May 7, 2020
0c5ac03
add changelog entry
MichaelBuessemeyer May 7, 2020
4663bf8
remove original element class and always send real element class to f…
May 13, 2020
ba6a2cf
merge master
May 13, 2020
5a1dc75
fix typo
May 13, 2020
aedfc6e
Merge branch 'master' of github.com:scalableminds/webknossos into com…
May 13, 2020
4b58075
add check so volumes can not be created for uint64 fallback layer
May 13, 2020
0116428
added manually saving of the originalElementClass in the frontend
MichaelBuessemeyer May 14, 2020
c892cbf
Merge branch 'master' of github.com:scalableminds/webknossos into com…
MichaelBuessemeyer May 14, 2020
04bd5ce
Merge branch 'master' of github.com:scalableminds/webknossos into com…
May 19, 2020
26086ec
update error message
May 19, 2020
3372fbd
Update frontend/javascripts/messages.js
MichaelBuessemeyer May 19, 2020
9b6594d
make comments more concise and remove unnecessary changes
MichaelBuessemeyer May 19, 2020
3951b3e
Merge branch 'master' into communicate-truncated-uint64
youri-k May 20, 2020
94aec77
Change "Show debug information" in toast to "Show more information"
daniel-wer May 20, 2020
8910068
Merge branch 'master' into communicate-truncated-uint64
youri-k May 27, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.released.md
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released
- Fixed a rendering error which could make some layers disappear in certain circumstances. [#4556](https://github.com/scalableminds/webknossos/pull/4556)
- Fixed a rendering error which caused negative float data to be rendered white. [#4556](https://github.com/scalableminds/webknossos/pull/4571)
- Fixed the histogram creation if some sampled positions don't contain data. [#4584](https://github.com/scalableminds/webknossos/pull/4584)
- Fixed a rendering exception which could occur in rare circumstandes. [#4588](https://github.com/scalableminds/webknossos/pull/4588)
- Fixed a rendering exception which could occur in rare circumstances. [#4588](https://github.com/scalableminds/webknossos/pull/4588)

## [20.04.0](https://github.com/scalableminds/webknossos/releases/tag/20.04.0) - 2020-03-23
[Commits](https://github.com/scalableminds/webknossos/compare/20.03.0...20.04.0)
Expand Down
2 changes: 2 additions & 0 deletions CHANGELOG.unreleased.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,10 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released
### Added

- Volume tracing data is now saved with lz4 compression, reducing I/O load and required disk space. [#4602](https://github.com/scalableminds/webknossos/pull/4602)
- Added a warning to the segmentation tab when viewing `uint64` bit segmentation data. [#4598](https://github.com/scalableminds/webknossos/pull/4598)

### Changed

- Improved the UI in navigation bar during loading of tracings and datasets. [#4612](https://github.com/scalableminds/webknossos/pull/4612)
- Improved logging in case of very slow annotation saving. Additionally, the user is also warned when there are unsaved changes older than two minutes. [#4593](https://github.com/scalableminds/webknossos/pull/4593)
- REST API for creating / changing datastores now contains additional field `allowsUpload` denoting if the datastore allows uploading datasets via browser. [#4614](https://github.com/scalableminds/webknossos/pull/4614)
Expand Down
43 changes: 29 additions & 14 deletions app/models/annotation/AnnotationService.scala
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import com.scalableminds.util.tools.{BoxImplicits, Fox, FoxImplicits, TextUtils}
import com.scalableminds.webknossos.tracingstore.SkeletonTracing._
import com.scalableminds.webknossos.tracingstore.VolumeTracing.{VolumeTracing, VolumeTracingOpt, VolumeTracings}
import com.scalableminds.webknossos.datastore.models.datasource.{
ElementClass,
DataSourceLike => DataSource,
SegmentationLayerLike => SegmentationLayer
}
Expand Down Expand Up @@ -102,18 +103,11 @@ class AnnotationService @Inject()(annotationInformationProvider: AnnotationInfor

private def createVolumeTracing(
dataSource: DataSource,
withFallback: Boolean,
fallbackLayer: Option[SegmentationLayer],
boundingBox: Option[BoundingBox] = None,
startPosition: Option[Point3D] = None,
startRotation: Option[Vector3D] = None
): VolumeTracing = {
val fallbackLayer: Option[SegmentationLayer] = if (withFallback) {
dataSource.dataLayers.flatMap {
case layer: SegmentationLayer => Some(layer)
case _ => None
}.headOption
} else None

): VolumeTracing =
VolumeTracing(
None,
boundingBoxToProto(boundingBox.getOrElse(dataSource.boundingBox)),
Expand All @@ -127,14 +121,21 @@ class AnnotationService @Inject()(annotationInformationProvider: AnnotationInfor
0,
VolumeTracingDefaults.zoomLevel
)
}

def createTracings(
dataSet: DataSet,
dataSource: DataSource,
tracingType: TracingType.Value,
withFallback: Boolean,
oldTracingId: Option[String] = None)(implicit ctx: DBAccessContext): Fox[(Option[String], Option[String])] =
oldTracingId: Option[String] = None)(implicit ctx: DBAccessContext): Fox[(Option[String], Option[String])] = {
def getFallbackLayer(): Option[SegmentationLayer] =
if (withFallback) {
dataSource.dataLayers.flatMap {
case layer: SegmentationLayer => Some(layer)
case _ => None
}.headOption
} else None

tracingType match {
case TracingType.skeleton =>
for {
Expand All @@ -148,16 +149,21 @@ class AnnotationService @Inject()(annotationInformationProvider: AnnotationInfor
case TracingType.volume =>
for {
client <- tracingStoreService.clientFor(dataSet)
volumeTracingId <- client.saveVolumeTracing(createVolumeTracing(dataSource, withFallback))
fallbackLayer = getFallbackLayer()
_ <- bool2Fox(fallbackLayer.forall(_.elementClass != ElementClass.uint64)) ?~> "annotation.volume.uint64"
volumeTracingId <- client.saveVolumeTracing(createVolumeTracing(dataSource, fallbackLayer))
} yield (None, Some(volumeTracingId))
case TracingType.hybrid =>
for {
client <- tracingStoreService.clientFor(dataSet)
fallbackLayer = getFallbackLayer()
_ <- bool2Fox(fallbackLayer.forall(_.elementClass != ElementClass.uint64)) ?~> "annotation.volume.uint64"
skeletonTracingId <- client.saveSkeletonTracing(
SkeletonTracingDefaults.createInstance.copy(dataSetName = dataSet.name, editPosition = dataSource.center))
volumeTracingId <- client.saveVolumeTracing(createVolumeTracing(dataSource, withFallback))
volumeTracingId <- client.saveVolumeTracing(createVolumeTracing(dataSource, fallbackLayer))
} yield (Some(skeletonTracingId), Some(volumeTracingId))
}
}

def createExplorationalFor(user: User, _dataSet: ObjectId, tracingType: TracingType.Value, withFallback: Boolean)(
implicit ctx: DBAccessContext): Fox[Annotation] =
Expand Down Expand Up @@ -338,9 +344,18 @@ class AnnotationService @Inject()(annotationInformationProvider: AnnotationInfor
dataSet <- dataSetDAO.findOneByNameAndOrganization(dataSetName, organizationId) ?~> Messages("dataset.notFound",
dataSetName)
dataSource <- dataSetService.dataSourceFor(dataSet).flatMap(_.toUsable)

fallbackLayer = if (volumeShowFallbackLayer) {
dataSource.dataLayers.flatMap {
case layer: SegmentationLayer => Some(layer)
case _ => None
}.headOption
} else None
_ <- bool2Fox(fallbackLayer.forall(_.elementClass != ElementClass.uint64)) ?~> "annotation.volume.uint64"

volumeTracing = createVolumeTracing(
dataSource,
withFallback = volumeShowFallbackLayer,
fallbackLayer = fallbackLayer,
boundingBox = boundingBox.flatMap { box =>
if (box.isEmpty) None else Some(box)
},
Expand Down
15 changes: 1 addition & 14 deletions app/models/binary/DataSetService.scala
Original file line number Diff line number Diff line change
Expand Up @@ -321,13 +321,12 @@ class DataSetService @Inject()(organizationDAO: OrganizationDAO,
lastUsedByUser <- lastUsedTimeFor(dataSet._id, requestingUserOpt)
dataStoreJs <- dataStoreService.publicWrites(dataStore)
dataSource <- dataSourceFor(dataSet, Some(organization), skipResolutions)
dataSourceWith64BitSupport = dataSource.toUsable.map(replaceUint64Layers).getOrElse(dataSource)
publicationOpt <- Fox.runOptional(dataSet._publication)(publicationDAO.findOne(_))
publicationJson <- Fox.runOptional(publicationOpt)(publicationService.publicWrites)
} yield {
Json.obj(
"name" -> dataSet.name,
"dataSource" -> dataSourceWith64BitSupport,
"dataSource" -> dataSource,
"dataStore" -> dataStoreJs,
"owningOrganization" -> organization.name,
"allowedTeams" -> teamsJs,
Expand All @@ -346,16 +345,4 @@ class DataSetService @Inject()(organizationDAO: OrganizationDAO,
"isForeign" -> dataStore.isForeign
)
}

private def replaceUint64Layers(dataSource: GenericDataSource[DataLayer]) = {
val newLayers = dataSource.dataLayers.map {
case l: WKWSegmentationLayer if l.elementClass == ElementClass.uint64 =>
l.copy(elementClass = ElementClass.uint32)
case l: AbstractSegmentationLayer if l.elementClass == ElementClass.uint64 =>
l.copy(elementClass = ElementClass.uint32)
case l => l
}

dataSource.copy(dataLayers = newLayers)
}
}
1 change: 1 addition & 0 deletions conf/messages
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ tracing=Annotation
tracing.notFound=Tracing couldn’t be found

annotation.create.failed=Failed to create annotation. This is likely because you are not a member of a team that has access to the dataset.
annotation.volume.uint64=Creating volume tracings on a uint64 fallback layer is not allowed.
annotation.notFound=Annotation couldn’t be found
annotation.notFound.considerLoggingIn=Annotation couldn’t be found. If the annotation is not public, you need to log in to see it.
annotation.invalid=Invalid annotation
Expand Down
1 change: 1 addition & 0 deletions frontend/javascripts/admin/api_flow_types.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ export type APISegmentationLayer = {|
...APIDataLayerBase,
+category: "segmentation",
+largestSegmentId: number,
+originalElementClass?: ElementClass,
+mappings?: Array<string>,
+agglomerates?: Array<string>,
+fallbackLayer?: ?string,
Expand Down
2 changes: 2 additions & 0 deletions frontend/javascripts/messages.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ instead. Only enable this option if you understand its effect. All layers will n
"You didn't add a node after jumping to this branchpoint, do you really want to jump again?",
"tracing.segmentation_zoom_warning":
"Segmentation data and volume annotation is only fully supported at a smaller zoom level.",
"tracing.uint64_segmentation_warning":
"This is an unsigned 64-bit segmentation. The displayed ids are truncated. Thus they might not match the ids on the server.",
MichaelBuessemeyer marked this conversation as resolved.
Show resolved Hide resolved
"tracing.segmentation_zoom_warning_agglomerate":
"Segmentation data which is mapped using an agglomerate file cannot be rendered in this magnification. Please zoom in further.",
"tracing.no_access": "You are not allowed to access this annotation.",
Expand Down
22 changes: 22 additions & 0 deletions frontend/javascripts/oxalis/model_initialization.js
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,24 @@ function initializeDataset(
dataSet: dataset.dataSource.id.name,
});

// Here we add the originalElementClass property to the segmentation layer if it exists.
Copy link
Member

Choose a reason for hiding this comment

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

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 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

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

// We also set the elementClass to uint32 because uint64 segmentation data is truncated to uint32 by the backend.
const updatedDataLayers = dataset.dataSource.dataLayers.map(dataLayer => {
const { elementClass } = dataLayer;
if (dataLayer.category === "segmentation") {
const adjustedElementClass = elementClass === "uint64" ? "uint32" : elementClass;
return {
...dataLayer,
originalElementClass: elementClass,
elementClass: adjustedElementClass,
};
} else {
return dataLayer;
}
});
// $FlowFixMe we assign the adjusted dataset layers, although this property is not writable.
dataset.dataSource.dataLayers = updatedDataLayers;

serverTracingAsVolumeTracingMaybe(tracing).map(volumeTracing => {
const newDataLayers = setupLayerForVolumeTracing(dataset, volumeTracing);
// $FlowFixMe We mutate the dataset here to avoid that an outdated version is used somewhere else
Expand Down Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

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

const tracingLayer = {
name: tracing.id,
elementClass: tracing.elementClass,
originalElementClass: tracing.elementClass,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

category: "segmentation",
largestSegmentId: tracing.largestSegmentId,
boundingBox: convertBoundariesToBoundingBox(boundaries),
Expand Down
20 changes: 18 additions & 2 deletions frontend/javascripts/oxalis/view/right-menu/mapping_info_view.js
Original file line number Diff line number Diff line change
Expand Up @@ -262,12 +262,28 @@ class MappingInfoView extends React.Component<Props, State> {
title,
dataIndex,
});
const showSegmentation64bitWarning =
segmentationLayer && segmentationLayer.originalElementClass === "uint64";
const maybeWithTooltipWarningTitle = title =>
showSegmentation64bitWarning ? (
<React.Fragment>
{title}{" "}
<Tooltip title={message["tracing.uint64_segmentation_warning"]}>
<Icon type="info-circle" style={{ color: "rgb(255, 155, 85)" }} />
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, that makes sense 🤯

</Tooltip>
</React.Fragment>
) : (
title
);
const idColumns =
hasMapping && this.props.isMappingEnabled
? // Show an unmapped and mapped id column if there's a mapping
[columnHelper("Unmapped", "unmapped"), columnHelper("Mapped", "mapped")]
[
columnHelper(maybeWithTooltipWarningTitle("Unmapped"), "unmapped"),
columnHelper(maybeWithTooltipWarningTitle("Mapped"), "mapped"),
]
: // Otherwise, only show an ID column
[columnHelper("ID", "unmapped")];
[columnHelper(maybeWithTooltipWarningTitle("ID"), "unmapped")];
const columns = [
columnHelper("", "name"),
...idColumns,
Expand Down