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

[python] Append-mode label/ID-mapping logic #1556

Merged
merged 13 commits into from
Aug 7, 2023
Merged

Conversation

johnkerl
Copy link
Member

@johnkerl johnkerl commented Jul 27, 2023

Issue and/or context:

Tracking issue: #1278

Changes:

  • Create the classes ExperimentAmbientLabelMapping, AxisAmbientLabelMapping, ExperimentIDMapping, and AxisIDMapping
  • None of these will be exposed to the user; they are internal-only.

Notes for Reviewer:

@johnkerl johnkerl force-pushed the kerl/append-mappings branch 4 times, most recently from a53a4dd to 39c7b5b Compare July 27, 2023 17:52
@codecov-commenter
Copy link

codecov-commenter commented Jul 27, 2023

Codecov Report

Patch coverage: 84.81% and project coverage change: +26.15% 🎉

Comparison is base (e26eeac) 64.82% compared to head (d682f92) 90.97%.

❗ 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     
Flag Coverage Δ
python 90.97% <84.81%> (+0.08%) ⬆️
r ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
...thon/src/tiledbsoma/io/registration/id_mappings.py 78.12% <78.12%> (ø)
...ledbsoma/io/registration/ambient_label_mappings.py 85.89% <85.89%> (ø)
.../python/src/tiledbsoma/io/registration/__init__.py 100.00% <100.00%> (ø)

... and 86 files with indirect coverage changes

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

@johnkerl johnkerl force-pushed the kerl/append-mappings branch 2 times, most recently from bec7375 to 674a41e Compare July 27, 2023 20:41
@johnkerl johnkerl changed the title [WIP] [python] Append-mode mapping classes [python] Append-mode mapping classes Jul 27, 2023
@johnkerl johnkerl marked this pull request as ready for review July 27, 2023 21:15
@johnkerl johnkerl changed the title [python] Append-mode mapping classes [python] Append-mode ID/label-mapping logic Jul 27, 2023
@johnkerl johnkerl changed the title [python] Append-mode ID/label-mapping logic [python] Append-mode label/ID-mapping logic Jul 27, 2023
Copy link
Contributor

@thetorpedodog thetorpedodog left a 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.

@johnkerl
Copy link
Member Author

johnkerl commented Aug 1, 2023

@thetorpedodog ready for round two! :)

@johnkerl
Copy link
Member Author

johnkerl commented Aug 4, 2023

@thetorpedodog ping 🙏

Copy link
Contributor

@thetorpedodog thetorpedodog left a 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!

@johnkerl
Copy link
Member Author

johnkerl commented Aug 7, 2023

@thetorpedodog ready for round three!

Copy link
Contributor

@thetorpedodog thetorpedodog left a 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.

@johnkerl
Copy link
Member Author

johnkerl commented Aug 7, 2023

@mojaveazure @aaronwolen thoughts? Again this is just a scaffolding PR, ingest-per-se coming as a follow-up ...

@johnkerl
Copy link
Member Author

johnkerl commented Aug 7, 2023

Happy to take further belated feedback on other PRs on #1278

@johnkerl johnkerl merged commit 7b37048 into main Aug 7, 2023
5 checks passed
@johnkerl johnkerl deleted the kerl/append-mappings branch August 7, 2023 20:30
beroy pushed a commit that referenced this pull request Aug 23, 2023
* [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`
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.

3 participants