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

Add nd support for tiff export job #7971

Merged
merged 16 commits into from
Sep 19, 2024
Merged

Add nd support for tiff export job #7971

merged 16 commits into from
Sep 19, 2024

Conversation

MichaelBuessemeyer
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer commented Aug 2, 2024

This PR enables tiff export for nd datasets.

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz -> not deployed as corresponding worker version is needed.

Steps to test:

  • Needs to be tested together with the worker of PR: https://github.com/scalableminds/voxelytics/pull/3667
  • Setup wk and worker locally
  • Test 3d dataset tiff export:
    • Choose your favourite 3D dataset, create an annotation and create a bbox
    • Open the download modal e.g. via the bbox dropdown menu and try to export the bbox as ome tiff and tiff stack zip
  • Test new nd dataset tiff export:
    • Choose your favourite ND dataset, create an annotation and create a bbox
    • Open the download modal e.g. via the bbox dropdown menu and try to export the bbox as ome tiff and tiff stack zip
  • All downloads should work

TODOs:

  • Nothing, hopefully :)

Issues:


(Please delete unneeded items, merge only when none are left open)

@MichaelBuessemeyer MichaelBuessemeyer changed the title WIP: Add nd support for tiff export job Add nd support for tiff export job Aug 27, 2024
@MichaelBuessemeyer MichaelBuessemeyer marked this pull request as ready for review August 27, 2024 13:14
@MichaelBuessemeyer
Copy link
Contributor Author

Hej, I managed to make the tiff export work on the worker side for nd datasets. Thus, this pr is ready for the first review round 👯

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.

the frontend lgtm 👍

@@ -97,6 +97,12 @@ case class FullAxisOrder(axes: Seq[Axis]) {
def permuteIndicesArrayToWk(indices: Array[Int]): Array[Int] =
arrayToWkPermutation.map(indices(_))

def toWkLibsDictObject: JsValue =
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
def toWkLibsDictObject: JsValue =
def toWkLibsDictJson: JsValue =

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also went for toWkLibsJson here as below to have the naming more consistent. Feel free to argue

@@ -70,6 +71,9 @@ case class BoundingBox(topLeft: Vec3Int, width: Int, height: Int, depth: Int) {
Vec3Int(width, height, depth)

def toLiteral: String = f"${topLeft.x},${topLeft.y},${topLeft.z},$width,$height,$depth"

def toWkLibsDict: JsObject =
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
def toWkLibsDict: JsObject =
def toWkLibsJson: JsObject =

@@ -10,6 +10,17 @@ case class AdditionalAxis(name: String, bounds: Array[Int], index: Int) {
lazy val lowerBound: Int = bounds(0)
lazy val upperBound: Int = bounds(1)
lazy val highestValue: Int = upperBound - 1

def enclosingAdditionalCoordinates(additionalCoordinates: Seq[AdditionalCoordinate]): AdditionalAxis = {
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment what this is for? I didn’t quite understand it on first read. Also, if it returns an Axis, why is it called enclosingAdditionalCoordinates? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed the method and added comments. Is it clearer now?

Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

I just tried testing this, but the »Export« button didn’t have any effect for me on this branch. Also no console logging. Works on master branch. Can you reproduce that? I’m on the l4_sample dataset

@MichaelBuessemeyer
Copy link
Contributor Author

MichaelBuessemeyer commented Sep 11, 2024

Can you reproduce that?

Nope sorry 🙈. I just tried again and it works for me with the l4_sample dataset. Maybe your bounding box was special regarding dimensions or yarn enable-jobs was missing or the worker not running?

@fm3
Copy link
Member

fm3 commented Sep 11, 2024

Huh, I re-tried and now it worked 🤔 Not sure what was different. Let’s assume it was a misconfiguration on my part.

One more thing I noticed: the bbox in the job list view at http://localhost:9000/jobs is no longer written out correctly.

By the way, I think 2d datasets should not be a problem (as they are handled as 3d with height=1 in wk)

@MichaelBuessemeyer
Copy link
Contributor Author

One more thing I noticed: the bbox in the job list view at http://localhost:9000/jobs is no longer written out correctly.

Thanks for the hint. Should be fixed now.

I also checked for backwards compatiblity that bboxes of old export jobs can be rendered.
image
The two top most jobs were made on the current master and the lower ones on this branch. Both render with the code of this branch

@MichaelBuessemeyer MichaelBuessemeyer merged commit 3d2a88e into master Sep 19, 2024
2 checks passed
@MichaelBuessemeyer MichaelBuessemeyer deleted the nd-tiff-export branch September 19, 2024 08:03
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.

Tiff export for ND data
3 participants