-
Notifications
You must be signed in to change notification settings - Fork 9
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
adding _annotation_mapping in AnalysisMixin #585
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #585 +/- ##
==========================================
+ Coverage 78.27% 78.52% +0.25%
==========================================
Files 36 36
Lines 3765 3828 +63
Branches 665 672 +7
==========================================
+ Hits 2947 3006 +59
- Misses 543 547 +4
Partials 275 275
|
src/moscot/base/problems/_mixins.py
Outdated
batch_size: Optional[int] = None, | ||
normalize: bool = True, | ||
scale_by_marginals: bool = True, | ||
): |
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.
missing return type
@giovp we would also wanna have this in the |
I think analysis mixin makes sense but as private method and then expose it only in relevant bio problems |
src/moscot/problems/space/_mixins.py
Outdated
@@ -562,6 +604,38 @@ def cell_transition( # type: ignore[misc] | |||
key_added=key_added, | |||
) | |||
|
|||
def celltype_mapping( |
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.
missing docstrings.
also introduce a variable "cell_transition_kwargs".
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.
"cell_transition_kwargs" should be default types.MappingProxyType({}). Raise error if cell_transition_kwargs
is not an empty dict
although cell_transition
is not used
please link to the example with the function. |
src/moscot/base/problems/_mixins.py
Outdated
scale_by_marginals: bool = True, | ||
): | ||
if mapping_mode == "sum": | ||
return self._cell_transition_online( |
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.
use _cell_transition
.
rename to |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
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.
regarding the comments in the docstring, please make sure that you change them everywhere (I only indicated in one of them)
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.
really great work @ArinaDanilina !!!! final polish and then LGTM!
Co-authored-by: Giovanni Palla <[email protected]>
for more information, see https://pre-commit.ci
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.
Thanks!
#569