Skip to content

Conversation

valentin-pinkau
Copy link
Member

@valentin-pinkau valentin-pinkau commented Aug 27, 2025

Description:

  • This PR adds get_agglomerate_graph the Annotation class.

Todos:

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

  • Updated Changelog
  • Added / Updated Tests

@valentin-pinkau valentin-pinkau self-assigned this Aug 27, 2025
Copy link

github-actions bot commented Sep 3, 2025

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
9424 7859 83% 80% 🟢

New Files

File Coverage Status
webknossos/webknossos/proofreading/_init_.py 100% 🟢
webknossos/webknossos/proofreading/agglomerate_graph.py 46% 🟢
webknossos/webknossos/proofreading/agglomerate_graph_pb2.py 28% 🟢
TOTAL 58% 🟢

Modified Files

File Coverage Status
webknossos/webknossos/annotation/annotation.py 79% 🟢
webknossos/webknossos/client/api_client/models.py 100% 🟢
webknossos/webknossos/client/api_client/tracingstore_api_client.py 48% 🟢
TOTAL 75% 🟢

updated for commit: 2667030 by action🐍

@valentin-pinkau valentin-pinkau requested a review from fm3 September 3, 2025 11:55
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.

Sweet stuff!

return file_path

def get_agglomerate_graph(self, agglomerate_id: int) -> AgglomerateGraph:
"""Get the agglomerate graph for the specified agglomerate id.
Copy link
Member

Choose a reason for hiding this comment

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

maybe note that this works only if the annotation is a proofreading annotation (aka has an editable mapping). Maybe also note that it works only for single-volume-layer annotations

AgglomerateGraph: The agglomerate graph for the specified agglomerate id.
Raises:
ValueError: If the agglomerate id is not valid.
Copy link
Member

Choose a reason for hiding this comment

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

Note that the tracingstore_client may also raise exceptions if the server response is not 200 or the response has an unexpected format

Comment on lines +48 to +51
protobuf_binary_response = self._get(route)
protobuf_binary = protobuf_binary_response.content
agglomerate_graph = AgglomerateGraph.from_proto(protobuf_binary)
return agglomerate_graph
Copy link
Member

Choose a reason for hiding this comment

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

Could the proto parsing be extracted to a helper method in the abstract_api_client? similar to _get_json, with agglomerate_graph_pb2.AgglomerateGraph as a type parameter?

AgglomerateGraph.fromProto could then not take the raw bytes but instead the already parsed proto?

@@ -0,0 +1,71 @@
// refresh with `protoc --python_out . webknossos/proofreading/agglomerate_graph.proto --pyi_out .`
Copy link
Member

Choose a reason for hiding this comment

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

This file will not be refreshed, though, is it? I think it is the source? Maybe the comment could be more explanatory.

Also, this file is copied from the webknossos repository, right? That should probably also be noted so that they don’t diverge.

@@ -0,0 +1,155 @@
from collections.abc import Iterable as _Iterable
Copy link
Member

Choose a reason for hiding this comment

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

could the generated files be in a folder named “generated” or similar, to mark this for other developers?

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