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

Annotation: support empty volumes & make volume location optional #814

Merged
merged 6 commits into from
Oct 20, 2022

Conversation

jstriebel
Copy link
Contributor

@jstriebel jstriebel commented Oct 19, 2022

Description:

This PR

Issues:

Todos:

  • Updated Changelog
  • Added / Updated Tests

@jstriebel jstriebel self-assigned this Oct 19, 2022
@jstriebel jstriebel changed the title [WIP] support empty volumes in temporary_volume_layer_copy(), make volume l… Annotation: support empty volumes & make volume location optional Oct 19, 2022
@jstriebel jstriebel requested a review from fm3 October 19, 2022 16:49
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.

LGTM; did not test myself. I assume the get_remote_dataset is now also possible for annotations saved with skip_volume_data? (Note that currently, those are zipped, despite only being a single nml file. In the future, we might opt to not zip those in the backend. would this still be compatible here?)

What is the benefit of changing the BoundingBox staticmethods to class methods? As far as I see it, cls is not used in the methods?

@jstriebel
Copy link
Contributor Author

LGTM; did not test myself. I assume the get_remote_dataset is now also possible for annotations saved with skip_volume_data?

I tested this with the nml_with_volumes.zip and .nml, this is downloaded from the actual wk PR, and also included in the tests.

(Note that currently, those are zipped, despite only being a single nml file. In the future, we might opt to not zip those in the backend. would this still be compatible here?)

Both would work now.

What is the benefit of changing the BoundingBox staticmethods to class methods? As far as I see it, cls is not used in the methods?

I should use cls for the constructor everywhere, changing this now. This ensures that when calling the method on subclasses (which we don't have atm), an instance of the subclass is created.

@jstriebel jstriebel enabled auto-merge (squash) October 20, 2022 11:13
@jstriebel jstriebel merged commit a7b4116 into master Oct 20, 2022
@jstriebel jstriebel deleted the optional-volume-locations-and-support-empty-volumes branch October 20, 2022 11:46
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.

annotation.temporary_volume_layer_copy() fails for empty volume annotations
2 participants