-
Notifications
You must be signed in to change notification settings - Fork 25
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
[python] Append-mode label/ID-mapping logic #1556
Conversation
a53a4dd
to
39c7b5b
Compare
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #1556 +/- ##
===========================================
+ Coverage 64.82% 90.97% +26.15%
===========================================
Files 105 34 -71
Lines 8788 2860 -5928
===========================================
- Hits 5697 2602 -3095
+ Misses 3091 258 -2833
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
bec7375
to
674a41e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was actually quite a bit easier to get through than I expected! Overall very well written and documented. I have a few minor suggestions but otherwise no concerns from me.
apis/python/src/tiledbsoma/io/registration/ambient_label_mappings.py
Outdated
Show resolved
Hide resolved
d24251c
to
bb4bcb5
Compare
@thetorpedodog ready for round two! :) |
0b83edd
to
ba1725f
Compare
@thetorpedodog ping 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few notes I think I might have missed before. Sorry for losing track of this!
apis/python/src/tiledbsoma/io/registration/ambient_label_mappings.py
Outdated
Show resolved
Hide resolved
apis/python/src/tiledbsoma/io/registration/ambient_label_mappings.py
Outdated
Show resolved
Hide resolved
apis/python/src/tiledbsoma/io/registration/ambient_label_mappings.py
Outdated
Show resolved
Hide resolved
apis/python/src/tiledbsoma/io/registration/ambient_label_mappings.py
Outdated
Show resolved
Hide resolved
ba1725f
to
0fa6afd
Compare
cf5c3de
to
1a88efc
Compare
4114916
to
c144bf9
Compare
@thetorpedodog ready for round three! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple small, optional suggestions, but this looks good to me overall. May still want to have a domain expert check it over but it looks like it does everything it should and stylistically it looks great.
@mojaveazure @aaronwolen thoughts? Again this is just a scaffolding PR, ingest-per-se coming as a follow-up ... |
Happy to take further belated feedback on other PRs on #1278 |
* [WIP] [python] Append-mode mapping classes * comment-block * dataclass -> attrs * more UT * code-review feedback * continue previous * code-review feedback * code-review feedback * code-review feedback * code-review feedback * code-review feedback * code-review feedback * Add `SOMATileDBContext`
Issue and/or context:
Tracking issue: #1278
Changes:
ExperimentAmbientLabelMapping
,AxisAmbientLabelMapping
,ExperimentIDMapping
, andAxisIDMapping
Notes for Reviewer: