Skip to content

Conversation

valentin-pinkau
Copy link
Member

@valentin-pinkau valentin-pinkau commented Sep 26, 2025

Description:

Todos:

Make sure to delete unnecessary points or to check all before merging:

  • Updated Changelog
  • Updated Documentation
  • Added / Updated Tests
  • Considered adding this to the Examples

@valentin-pinkau valentin-pinkau self-assigned this Sep 26, 2025

def write_ome_metadata(dataset: "Dataset", layer: "Layer") -> None:
if layer.path is None or not is_writable_path(layer.path):
assert layer.path is not None
Copy link
Member Author

Choose a reason for hiding this comment

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

no assert needed anymore

Copy link
Member

@normanrz normanrz left a comment

Choose a reason for hiding this comment

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

Looks great! I think we should track how backwards-compatible this change is. The tests will probably tell us.

return dataset_bbox

@staticmethod
def get_remote_datasets(
Copy link
Member

Choose a reason for hiding this comment

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

This means it is both on Dataset and RemoteDataset?

def __init__(self, layer: "SegmentationLayer", properties: "AttachmentsProperties"):
super().__init__(layer, properties)

def _get_optional_dataset_path(self) -> UPath | None:
Copy link
Member

Choose a reason for hiding this comment

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

Is this actually optional?

return dataset

@classmethod
def open_remote(
Copy link
Member

Choose a reason for hiding this comment

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

Should this and download also be moved to AbstractDataset as get_remote_datasets?


@abstractmethod
@property
def attachments_type(self) -> type[AttachmentsTypeT]:
Copy link
Member

Choose a reason for hiding this comment

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

protected?

Suggested change
def attachments_type(self) -> type[AttachmentsTypeT]:
def _attachments_type(self) -> type[AttachmentsTypeT]:

def dataset(self) -> RemoteDataset:
return self._dataset

def to_layer_to_link(self) -> LayerToLink:
Copy link
Member

Choose a reason for hiding this comment

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

This feels unnessecary. Couldn't the function that takes LayerToLink take a RemoteLayer instead?

"""

_layer: "Layer"
_layer: "AbstractLayer"
Copy link
Member

Choose a reason for hiding this comment

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

Would it help to make MagView generic over the Layer class?



class RemoteDataset(AbstractDataset[RemoteLayer, RemoteSegmentationLayer]):
"""A representation of a dataset on a webknossos server.
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
"""A representation of a dataset on a webknossos server.
"""A representation of a dataset managed by a WEBKNOSSOS server.

"""Move the dataset to a folder. Specify the folder like this `RemoteFolder.get_by_path("Datasets/Folder_A")`."""
self._update_dataset_info(folder_id=folder.id)

def download_mesh(
Copy link
Member

Choose a reason for hiding this comment

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

Out of scope for this PR, but this need to move. Probably to the RemoteSegmentationLayer.

@normanrz
Copy link
Member

normanrz commented Oct 1, 2025

Please also make sure that the docs work for the new classes.

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.

2 participants