Skip to content

Feature/hucira#901

Open
Jenniliu12 wants to merge 12 commits into
scverse:mainfrom
Jenniliu12:feature/hucira
Open

Feature/hucira#901
Jenniliu12 wants to merge 12 commits into
scverse:mainfrom
Jenniliu12:feature/hucira

Conversation

@Jenniliu12
Copy link
Copy Markdown

@Jenniliu12 Jenniliu12 commented Jan 11, 2026

PR Checklist

  • Referenced issue is linked
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated

Description of changes
Removed hucira module; instead only implemented loading function.

Technical details
No technical changes. The analysis reuses methods from the existing enrichment module and scanpy. An example of use case is added to the enrichment tutorial.

Additional context
No tests added.
Tutorial not updated yet.

@Jenniliu12 Jenniliu12 marked this pull request as draft January 12, 2026 22:20
Copy link
Copy Markdown
Member

@Zethson Zethson left a comment

Choose a reason for hiding this comment

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

Thank you very much already! Here's some first initial feedback:

  1. Please ensure that you have a descriptive pull request title and the pull request description only has the necessary detail. This can also be done later but it's important.
  2. Your implementation should not add any dependencies. Pertpy has to minimize dependencies because it's used by a loooot of people. Every dependency that we add has the potential to break users environment and increases complexity. Therefore, please remove bokeh, pycirclize, and tqdm. The plot should be implemented with seaborn and instead of using tqdm, you can use Rich directly. Not sure what to do about pycirclize - if necessary we can talk about it. That's one of the differences of implementing something for yourself vs implementing it for many.
  3. Please ensure that all functions that have a bit more than trivial complexity have a proper docstring. Please adhere to our docstring style.
  4. I didn't review everything yet because I think there's still a lot to do. Let's address these first.

Comment thread pertpy/data/_datasets.py Outdated
Comment thread pertpy/data/_datasets.py Outdated
Comment thread pertpy/data/_datasets.py Outdated
Comment thread pertpy/data/_datasets.py
Comment thread pertpy/data/_datasets.py Outdated
Comment thread pertpy/tools/_hucira.py Outdated
2. Creates ranking of query data genes contrasting condition1 vs condition2. A continuum from genes most associated with condition1 (top) to genes most associated with condition2 (bottom)
3. Computes enrichment of each cytokine by matching their associated gene set in the ranked list.

Parameters
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

docstring

Comment thread pertpy/tools/_hucira.py Outdated
verbose: bool = False,
threads: int = 6,
) -> pd.DataFrame:
"""Function wrapper: Computes cytokine enrichment activity in one celltype using GSEA scoring. Loops through several threshold value to obtain more robust gene sets.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
"""Function wrapper: Computes cytokine enrichment activity in one celltype using GSEA scoring. Loops through several threshold value to obtain more robust gene sets.
"""Computes cytokine enrichment activity in one celltype using GSEA scoring.
Loops through several threshold value to obtain more robust gene sets.

It's a rule that the first line is always just one sentence. No exceptions.

Comment thread pertpy/tools/_hucira.py Outdated
weight: float = 1.0,
seed: int = 2025,
verbose: bool = False,
threads: int = 6,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a super random value

Comment thread tests/tools/test_hucira.py Outdated
ranked_stats, _num_cells = hucira._compute_ranking_statistic(dummy_adata, contrast_column, contrasts_combo)
assert isinstance(ranked_stats, pd.DataFrame)

# with pytest.raises(KeyError):
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This shouldn't be commented out, right?

Comment thread tests/tools/test_hucira.py Outdated
("B cell", "B_cell"),
("CD8a", "CD8_T_cell"),
("Mono", "CD14_Mono"),
] # can't be a list for "run_one_enrichment_test()"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure what's meant with that.

@Zethson
Copy link
Copy Markdown
Member

Zethson commented Mar 9, 2026

@Jenniliu12 are you going to follow up with this PR soon? Do you need some more comments?
Just want to ensure that I'm not blocking anything.

@Zethson
Copy link
Copy Markdown
Member

Zethson commented May 19, 2026

@Jenniliu12 are you still going to work on this PR or should we close it?

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 20, 2026

Codecov Report

❌ Patch coverage is 15.38462% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.26%. Comparing base (12897e1) to head (f253cc8).
⚠️ Report is 80 commits behind head on main.

Files with missing lines Patch % Lines
pertpy/data/_datasets.py 15.38% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #901      +/-   ##
==========================================
+ Coverage   73.54%   77.26%   +3.72%     
==========================================
  Files          48       48              
  Lines        5613     6229     +616     
==========================================
+ Hits         4128     4813     +685     
+ Misses       1485     1416      -69     
Files with missing lines Coverage Δ
pertpy/data/__init__.py 100.00% <ø> (ø)
pertpy/data/_datasets.py 42.10% <15.38%> (-57.90%) ⬇️

... and 10 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Jenniliu12
Copy link
Copy Markdown
Author

Tutorials PR: scverse/pertpy-tutorials#59 is ready for review. I think it needs to be merged into the scverse/pertpy-tutorials main branch first before I can update the submodule in here, is this correct?

@Zethson
Copy link
Copy Markdown
Member

Zethson commented May 25, 2026

Tutorials PR: scverse/pertpy-tutorials#59 is ready for review. I think it needs to be merged into the scverse/pertpy-tutorials main branch first before I can update the submodule in here, is this correct?

Actually, no. You can update the submodule in this PR to point to your branch.

@Jenniliu12 Jenniliu12 marked this pull request as ready for review May 25, 2026 16:16
@Jenniliu12
Copy link
Copy Markdown
Author

Just updated the submodule. Does this build fail have anything to do with my changes? Claude says "it's a pre-existing pertpy docs config problem", but do I need to do anything else now or is it really ready for review now? :)

Comment thread .gitmodules Outdated
[submodule "docs/notebooks"]
path = docs/tutorials/notebooks
url = https://github.com/scverse/pertpy-tutorials/
url = https://github.com/Jenniliu12/pertpy-tutorials/
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do not change this, please! This is also not described in the contributing guide

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Okay got it, sorry!

Can I please have some additional guidance on the correct steps? I could not find specific information about submodules in the contributing guide. Can you please point me to the specific section in the docs? (I followed steps recommended by claude, and I was misguided)

My current understanding:

  1. I revert the .gitmodules url back to the original https://github.com/scverse/pertpy-tutorials/ ,
  2. My only change is pointing the SHA in the submodule directory to the tutorial commit in my pertpy-tutorials branch (Is this all you meant with "update the submodule in this PR to point to your branch")

@Zethson
Copy link
Copy Markdown
Member

Zethson commented May 26, 2026

Just updated the submodule. Does this build fail have anything to do with my changes? Claude says "it's a pre-existing pertpy docs config problem", but do I need to do anything else now or is it really ready for review now? :)

It's not ready for a review.

  1. The PR has merge conflicts because the submodule pointer isn't right.
  2. The pre-commit checks fail.
  3. The ReadTheDocs build fails.

Please take some time to understand what's happening. I'm always happy to help. The contributing guide is your friend.

@Zethson
Copy link
Copy Markdown
Member

Zethson commented May 28, 2026

We're getting there 😄
However, the cytokine file is not a dataset. It belongs to the metadata module and should be there.

Concerning the tutorial:

try:
    import matplotlib.pyplot as plt
    import seaborn as sns
except ImportError as e:
    raise ImportError(
        f"Plotting requires matplotlib and seaborn ({e.name} not found). "
        "Install these separately."
    ) from e

Please remove this.

Please also ensure that sentences in markdown cells are on their own lines for easier reviews.

Thanks!

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