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.

Sign up for GitHub

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 expected node type from server by renaming resolution to mag #8187

Merged
merged 2 commits into from
Nov 12, 2024
Merged

Fix expected node type from server by renaming resolution to mag #8187

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
278b548
fix server node type by switching back to mag
Nov 12, 2024
de63a3a
add changelog entry
Nov 12, 2024
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
1 change: 1 addition & 0 deletions CHANGELOG.unreleased.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released
- Deleting a bounding box is now possible independently of a visible segmentation layer. [#8164](https://github.com/scalableminds/webknossos/pull/8164)
- S3-compliant object storages can now be accessed via HTTPS. [#8167](https://github.com/scalableminds/webknossos/pull/8167)
- Fixed that skeleton tree nodes were created with the wrong mag. [#8185](https://github.com/scalableminds/webknossos/pull/8185)
- Fixed the expected type of a tree node received from the server. Fixes nml export to include the `inMag` field correctly. [#8187](https://github.com/scalableminds/webknossos/pull/8187)
- Fixed a layout persistence bug leading to empty viewports, triggered when switching between orthogonal, flight, or oblique mode. [#8177](https://github.com/scalableminds/webknossos/pull/8177)

### Removed
Expand Down
2 changes: 1 addition & 1 deletion frontend/javascripts/oxalis/model/helpers/generate_dummy_trees.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export default function generateDummyTrees(
},
radius: 112.39999389648438,
viewport: 1,
resolution: 1,
mag: 1,
bitDepth: 4,
interpolation: true,
createdTimestamp: 1507550793899,
Expand Down
2 changes: 1 addition & 1 deletion frontend/javascripts/oxalis/model/reducers/skeletontracing_reducer_helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -846,7 +846,7 @@ function serverNodeToMutableNode(n: ServerNode): MutableNode {
rotation: Utils.point3ToVector3(n.rotation),
bitDepth: n.bitDepth,
viewport: n.viewport,
resolution: n.resolution,
resolution: n.mag,
radius: n.radius,
timestamp: n.createdTimestamp,
interpolation: n.interpolation,
Expand Down
6 changes: 3 additions & 3 deletions frontend/javascripts/test/fixtures/skeletontracing_server_objects.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ export const tracing: ServerSkeletonTracing = {
},
radius: 112.39999389648438,
viewport: 0,
resolution: 1,
mag: 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Residual resolution fields found in type definitions.

Several type definitions still contain resolution fields. Please update these to maintain consistency.

  • frontend/javascripts/types/api_flow_types.ts: readonly resolutions: Array<Vector3>;
  • frontend/javascripts/test/reducers/skeletontracing_reducer.spec.ts: resolution: 10,
  • frontend/javascripts/oxalis/shaders/main_data_shaders.glsl.ts: resolutionsCount: number;
  • frontend/javascripts/router.tsx: const resolutionRestrictions: APIMagRestrictions = {};
  • frontend/javascripts/oxalis/view/left-border-tabs/layer_settings_tab.tsx: const getMaxDim = (resolution: Vector3) => Math.max(...resolution);
  • frontend/javascripts/oxalis/view/right-border-tabs/connectome_tab/connectome_view.tsx: resolution: 0,
  • frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx: handleQualityChangeForPrecomputation = (resolutionIndex: number) =>
  • frontend/javascripts/oxalis/view/right-border-tabs/segments_tab/segments_view.tsx: handleQualityChangeForAdHocGeneration = (resolutionIndex: number) =>
  • frontend/javascripts/oxalis/view/jobs/train_ai_model.tsx: resolutions ? resolutions.map(Utils.point3ToVector3) : ([[1, 1, 1]] as Vector3[]),
  • frontend/javascripts/oxalis/workers/async_bucket_picker.worker.ts: resolutions: Array<Vector3>,
  • frontend/javascripts/test/model/model_resolutions.spec.ts: resolutions: [
  • frontend/javascripts/oxalis/model/accessors/volumetracing_accessor.ts: resolution: Vector3;
  • frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts: const resolutionUnionDict: { [key: number]: Vector3[] } = {};
  • frontend/javascripts/oxalis/model/accessors/flycam_accessor.ts: resolutionIdentifier: number,
  • frontend/javascripts/test/fixtures/volumetracing_object.ts: // @ts-expect-error ts-migrate(2322) FIXME: Type '{ resolutions: [number, number, number][]; c...
  • frontend/javascripts/oxalis/model/helpers/position_converter.ts: const resolution = magIndex < mags.length ? mags[magIndex] : upsampleMag(mags, magIndex);
  • frontend/javascripts/types/schemas/datasource.schema.ts: resolution: {
  • frontend/javascripts/types/schemas/datasource.types.ts: resolution: number | Vector3;
  • frontend/javascripts/admin/admin_rest_api.ts: resolution: Vector3 | null | undefined;
🔗 Analysis chain

LGTM! Property rename aligns with server behavior.

The change from resolution to mag correctly reflects the server's behavior as mentioned in the PR objectives. The modification is consistent across all nodes and maintains the existing values.

Let's verify the type definitions to ensure consistency:

Also applies to: 73-73, 93-93

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that the type definitions have been updated to use 'mag' instead of 'resolution'
# Expected: Should find type definitions using 'mag' and no occurrences of 'resolution' in node types

# Search for node type definitions
ast-grep --pattern 'type $_ = {
  $$$
  mag: $_
  $$$
}'

# Verify no lingering 'resolution' fields in type definitions
rg 'resolution.*:' -g '*.ts' -g '*.tsx' -g '*.d.ts'

Length of output: 11629

bitDepth: 4,
interpolation: true,
createdTimestamp: 1502302785450,
Expand Down Expand Up @@ -70,7 +70,7 @@ export const tracing: ServerSkeletonTracing = {
},
radius: 112.39999389648438,
viewport: 0,
resolution: 1,
mag: 1,
bitDepth: 4,
interpolation: true,
createdTimestamp: 1502302785447,
Expand All @@ -90,7 +90,7 @@ export const tracing: ServerSkeletonTracing = {
},
radius: 112.39999389648438,
viewport: 0,
resolution: 1,
mag: 1,
bitDepth: 4,
interpolation: true,
createdTimestamp: 1502302785448,
Expand Down
2 changes: 1 addition & 1 deletion frontend/javascripts/test/fixtures/tasktracing_server_objects.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ export const tracing: ServerSkeletonTracing = {
},
radius: 120,
viewport: 1,
resolution: 1,
mag: 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Incomplete renaming of resolution to mag in test fixtures.

Please update the following test fixtures to replace resolution with mag:

  • frontend/javascripts/test/fixtures/volumetracing_object.ts
  • frontend/javascripts/test/fixtures/dataset_server_object.ts
🔗 Analysis chain

LGTM! Property name change aligns with server response format.

The change from resolution to mag correctly reflects the server's node field naming, ensuring test fixtures match the actual API response structure.

Let's verify if similar changes were made in all test fixtures:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining instances of 'resolution' in test fixtures
# and confirm consistent use of 'mag' property in node definitions

# Search for any remaining 'resolution' properties in test fixtures
echo "Checking for remaining 'resolution' properties in test fixtures:"
rg -l "resolution.*:" "frontend/javascripts/test/fixtures/"

# Search for node definitions to verify 'mag' property usage
echo "Verifying 'mag' property usage in node definitions:"
rg -A 5 "nodes.*:.*\[" "frontend/javascripts/test/fixtures/"

Length of output: 2032

bitDepth: 0,
interpolation: false,
createdTimestamp: 1528811979356,
Expand Down
2 changes: 1 addition & 1 deletion frontend/javascripts/types/api_flow_types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -756,7 +756,7 @@ export type ServerNode = {
rotation: Point3;
bitDepth: number;
viewport: number;
resolution: number;
mag: number;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the root of the problem. The backend sends nodes with the field called mag and not resolution. See:

webknossos/webknossos-datastore/proto/SkeletonTracing.proto

Lines 8 to 19 in 8dec578

message Node {
required int32 id = 1;
required Vec3IntProto position = 2;
required Vec3DoubleProto rotation = 3;
required float radius = 4;
required int32 viewport = 5;
required int32 mag = 6;
required int32 bitDepth = 7;
required bool interpolation = 8;
required int64 createdTimestamp = 9;
repeated AdditionalCoordinateProto additionalCoordinates = 10;
}

By changing the type, TS should show all locations to change and the resulting code should be fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, that makes sense!

radius: number;
createdTimestamp: number;
interpolation: boolean;
Expand Down