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

refactor: different pattern for structured annotations list and shapes union #237

Merged
merged 26 commits into from
Dec 28, 2023

Conversation

tlambert03
Copy link
Owner

@tlambert03 tlambert03 commented Dec 17, 2023

This PR improves the implementation of StructuredAnnotations and ShapeUnion.

These have been difficult objects to capture in a way that feels natural in a python context, while remaining true to the semantics of the original OME schema.

In ome-types v0.4, these objects both became list-like objects that removed the individual sub-fields. That is, they looked a bit more like this:

class OME(BaseModel):
    structured_annotions: list[XMLAnnotation | FileAnnotation | ListAnnotation] = []

than this:

class StructuredAnnotations(BaseModel):
    xml_annotations: list[XMLAnnotation]
    file_annotations: list[FileAnnotation]
    list_annotations: list[ListAnnotation]

class OME(BaseModel):
    structured_annotions: StructuredAnnotations | None = None

However, that came a bit at the expense of "correctness", and it also made the casting to JSON schema a bit clunky as well (which was recently fixed in a somewhat hacky way in #235).

This PR moves back towards the second pattern: a type StructuredAnnotations that does indeed have individual fields, matching the original XSD more faithfully... but it adds methods to that object that implement most (but not all) of the MutableSequence interface. That is, you can still use append, extend, remove, __getitem__ (but not __setitem__) on the object itself.

It's a significant internal refactor, but hopefully shouldn't break any external use cases (and the third-party tests for aicsimageio, omero-cli-transfer, and paquo are all still working after this PR).

Copy link

codecov bot commented Dec 17, 2023

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (af72da7) 86.33% compared to head (4f5718f) 86.75%.

Files Patch % Lines
src/ome_types/model/__init__.py 45.45% 6 Missing ⚠️
src/ome_types/_mixins/_collections.py 96.55% 2 Missing ⚠️
src/ome_autogen/_generator.py 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #237      +/-   ##
==========================================
+ Coverage   86.33%   86.75%   +0.41%     
==========================================
  Files          26       24       -2     
  Lines        1259     1200      -59     
==========================================
- Hits         1087     1041      -46     
+ Misses        172      159      -13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

codspeed-hq bot commented Dec 17, 2023

CodSpeed Performance Report

Merging #237 will improve performances by 25.59%

Comparing better-structured-annotations (4f5718f) with main (af72da7)

Summary

⚡ 1 improvements
✅ 8 untouched benchmarks

Benchmarks breakdown

Benchmark main better-structured-annotations Change
test_time_to_xml[large] 8 s 6.3 s +25.59%

@tlambert03 tlambert03 changed the title refactor: different pattern for structured annotations list refactor: different pattern for structured annotations list and shapes union Dec 18, 2023
@joshmoore
Copy link
Collaborator

Wow! That looks like it was painful, @tlambert03. Thank you!

I'm not sure I will be able to test before disappearing for the holidays, but it's definitely on my list.

@tlambert03 tlambert03 merged commit 80e4b6a into main Dec 28, 2023
34 checks passed
@tlambert03 tlambert03 deleted the better-structured-annotations branch December 28, 2023 17:12
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