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

Mag information is not lost on empty volumes #6481

Merged
merged 7 commits into from
Sep 22, 2022

Conversation

frcroth
Copy link
Member

@frcroth frcroth commented Sep 20, 2022

URL of deployed dev instance (used for testing):

Steps to test:

  • Select a dataset with multiple mags
  • Create a volume with fewer mags
  • Download that volume
  • Reload the dataset
  • Upload the volume
  • Ensure that only the selected mags are available in the volume

Issues:

The original handling is still largely there to handle previously exported files.


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

@frcroth frcroth requested a review from fm3 September 21, 2022 12:11
@frcroth frcroth changed the title WIP: Mag information is not lost on empty volumes Mag information is not lost on empty volumes Sep 21, 2022
@frcroth frcroth marked this pull request as ready for review September 21, 2022 12:13
@frcroth frcroth self-assigned this Sep 21, 2022
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.

Looking good :) Please see my small comment on Vec3Int

@@ -22,6 +22,7 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released
- Selecting a node with the proofreading tool won't have any side effects anymore. Previous versions could load additional agglomerate skeletons in certain scenarios which could be confusing. [#6477](https://github.com/scalableminds/webknossos/pull/6477)
- Sharing links are shortened by default. Within the sharing modal, this shortening behavior can be disabled. [#6461](https://github.com/scalableminds/webknossos/pull/6461)
- Removed optional "resolution" parameter from /datasets/:organizationName/:dataSetName/layers/:dataLayerName/data route. Use mag instead. [#6479](https://github.com/scalableminds/webknossos/pull/6479)
- Changed how volumes containing no data are stored. Now the selection of magnitudes is correctly exported and imported. [#6481](https://github.com/scalableminds/webknossos/pull/6481)
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
- Changed how volumes containing no data are stored. Now the selection of magnitudes is correctly exported and imported. [#6481](https://github.com/scalableminds/webknossos/pull/6481)
- Changed how volumes containing no data are stored. Now the selection of magnifications is correctly exported and imported. [#6481](https://github.com/scalableminds/webknossos/pull/6481)

import com.scalableminds.webknossos.wrap.{BlockType, WKWFile, WKWHeader}

import scala.collection.mutable
import scala.concurrent.Future

class WKWBucketStreamSink(val layer: DataLayer) extends WKWDataFormatHelper {

def apply(bucketStream: Iterator[(BucketPosition, Array[Byte])]): Iterator[NamedStream] = {
def apply(bucketStream: Iterator[(BucketPosition, Array[Byte])], mags: Seq[Vec3IntProto]): Iterator[NamedStream] = {
Copy link
Member

Choose a reason for hiding this comment

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

I’d suggest passing a Vec3Int in here, (not the proto version). The proto version does not have all the nice convenience methods we use all around the scala code. You can have the caller extend the ProtoGeometryImplicits trait, which should make vec3IntFromProto available (and since it is implicit, you might not even have to call it if the function signatures make the types unambiguous)

@frcroth frcroth requested a review from fm3 September 22, 2022 09:47
@frcroth frcroth merged commit b40b75a into master Sep 22, 2022
@frcroth frcroth deleted the empty-volumes-download-upload branch September 22, 2022 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make mag list detection for volume uploads more resilient
2 participants