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

adding _annotation_mapping in AnalysisMixin #585

Merged
merged 85 commits into from
Jan 19, 2024
Merged

Conversation

ArinaDanilina
Copy link
Collaborator

@codecov
Copy link

codecov bot commented Jul 19, 2023

Codecov Report

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

Comparison is base (924c78a) 78.27% compared to head (0cd1ae5) 78.52%.

Additional details and impacted files

Impacted file tree graph

@@            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              
Files Coverage Δ
src/moscot/base/problems/_utils.py 63.69% <0.00%> (ø)
src/moscot/problems/cross_modality/_mixins.py 83.60% <83.33%> (-0.33%) ⬇️
src/moscot/problems/time/_mixins.py 63.26% <88.88%> (+0.50%) ⬆️
src/moscot/utils/subset_policy.py 65.04% <0.00%> (+0.97%) ⬆️
src/moscot/base/problems/_mixins.py 84.83% <95.74%> (+2.69%) ⬆️
src/moscot/problems/space/_mixins.py 81.64% <77.77%> (-0.15%) ⬇️

batch_size: Optional[int] = None,
normalize: bool = True,
scale_by_marginals: bool = True,
):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing return type

@MUCDK
Copy link
Collaborator

MUCDK commented Jul 20, 2023

@giovp we would also wanna have this in the TranslationProblem. For the temporalProblem I don't know how much sense this makes. Users should not misinterpret, but maybe still consider putting this into the AnalysisMixin?

@giovp
Copy link
Member

giovp commented Jul 20, 2023

@giovp we would also wanna have this in the TranslationProblem. For the temporalProblem I don't know how much sense this makes. Users should not misinterpret, but maybe still consider putting this into the AnalysisMixin?

I think analysis mixin makes sense but as private method and then expose it only in relevant bio problems

@@ -562,6 +604,38 @@ def cell_transition( # type: ignore[misc]
key_added=key_added,
)

def celltype_mapping(
Copy link
Collaborator

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".

Copy link
Collaborator

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

@MUCDK
Copy link
Collaborator

MUCDK commented Jul 20, 2023

please link to the example with the function.

scale_by_marginals: bool = True,
):
if mapping_mode == "sum":
return self._cell_transition_online(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use _cell_transition.

@MUCDK
Copy link
Collaborator

MUCDK commented Jul 20, 2023

rename to annotation_mapping

@ArinaDanilina ArinaDanilina linked an issue Jul 26, 2023 that may be closed by this pull request
@ArinaDanilina ArinaDanilina changed the title adding _celltype_mapping in AnalysisMixin adding _annotation_mapping in AnalysisMixin Aug 3, 2023
Copy link
Collaborator

@MUCDK MUCDK left a 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)

Copy link
Member

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

src/moscot/base/problems/_mixins.py Outdated Show resolved Hide resolved
src/moscot/problems/cross_modality/_mixins.py Show resolved Hide resolved
src/moscot/problems/cross_modality/_mixins.py Outdated Show resolved Hide resolved
src/moscot/problems/space/_mixins.py Outdated Show resolved Hide resolved
src/moscot/problems/space/_mixins.py Outdated Show resolved Hide resolved
src/moscot/problems/space/_mixins.py Show resolved Hide resolved
src/moscot/problems/time/_mixins.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@MUCDK MUCDK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@ArinaDanilina ArinaDanilina merged commit 7a4883e into main Jan 19, 2024
8 checks passed
@ArinaDanilina ArinaDanilina deleted the add/celltype_mapping branch January 19, 2024 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants