Conversation
- Add tests/test_allele_utils.py and tests/test_filter.py - Update polyase/allele_utils.py - Bump version to 1.3.3 in __init__.py and pyproject.toml Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Automatically creates a GitHub Release with generated release notes when a v*.*.* tag is pushed. This in turn triggers the existing python-publish.yml workflow to publish to PyPI. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Reviewer's GuideAdds extensive unit test coverage for the filtering and allele_utils functionality, fixes the AlleleRatioCalculator.get_ratios_for_synt_id orientation bug, bumps the package version, and introduces a GitHub Actions workflow for tagged release automation. Class diagram for AlleleRatioCalculator get_ratios_for_synt_id changeclassDiagram
class AlleleRatioCalculator {
- adata
+ get_ratios_for_synt_id(synt_id, ratio_layer)
}
class AnnData {
+ var
+ layers
}
class VarTable {
+ Synt_id
}
class Layers {
+ allelic_ratio_unique_counts
+ unique_counts
}
AlleleRatioCalculator --> AnnData : uses
AnnData --> VarTable : has
AnnData --> Layers : has
note for AlleleRatioCalculator "get_ratios_for_synt_id now indexes layers as [:, mask] instead of [mask] to return ratios across all cells for a given Synt_id"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/test_allele_utils.py" line_range="314-323" />
<code_context>
+class TestGetRatiosForSyntId:
</code_context>
<issue_to_address>
**question (testing):** Add a test for get_ratios_for_synt_id when the requested Synt_id does not exist.
There’s no test for how `get_ratios_for_synt_id` behaves when the requested `synt_id` is missing from `adata.var['Synt_id']`. Please add a test that locks in the expected behaviour (e.g. raising a clear `ValueError` vs returning an empty array) so this edge case is covered against future regressions.
</issue_to_address>
### Comment 2
<location path="tests/test_filter.py" line_range="460-141" />
<code_context>
+class TestMinExpressionTypes:
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for library_size_dependent combined with non-callable thresholds.
You already cover scalar, dict, and callable `min_expression` values, plus `library_size_dependent=True` for the callable case. Please also add a test for `library_size_dependent=True` with a non-callable threshold (scalar or dict), asserting either the expected behaviour or a clear error. This will document and fix the intended semantics of that parameter combination.
Suggested implementation:
```python
class TestMinExpressionTypes:
def test_float_threshold(self, adata):
result = filter_low_expressed_genes(
adata, min_expression=10.0, lib_size_normalization=None, verbose=False
)
assert result.n_vars == 2
def test_scalar_threshold_with_library_size_dependent_raises(self, adata):
# Non-callable thresholds are not compatible with library_size_dependent=True
with pytest.raises(ValueError):
filter_low_expressed_genes(
adata,
min_expression=10.0,
lib_size_normalization=None,
library_size_dependent=True,
verbose=False,
)
def test_dict_threshold_with_library_size_dependent_raises(self, adata):
# Non-callable dict thresholds are not compatible with library_size_dependent=True
threshold = {"A": 10.0, "B": 5.0}
with pytest.raises(ValueError):
filter_low_expressed_genes(
adata,
min_expression=threshold,
lib_size_normalization=None,
library_size_dependent=True,
verbose=False,
)
def test_dict_threshold_per_sample(self, adata):
"""
Per-sample thresholds.
Group A sums: s1=300, s2=250, s3=250.
```
1. Ensure `pytest` is imported at the top of `tests/test_filter.py` if it is not already:
- `import pytest`
2. Update the implementation of `filter_low_expressed_genes` (where it lives) so that when `library_size_dependent=True` and `min_expression` is not callable (i.e. scalar or dict), it raises a `ValueError` matching these tests’ expectations.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+314
to
+323
| class TestGetRatiosForSyntId: | ||
| def test_raises_when_ratio_layer_missing(self, adata_two_groups): | ||
| calc = AlleleRatioCalculator(adata_two_groups) | ||
| with pytest.raises(ValueError, match="not found"): | ||
| calc.get_ratios_for_synt_id(1) | ||
|
|
||
| def test_returns_correct_values_for_synt_id(self, adata_two_groups): | ||
| calc = AlleleRatioCalculator(adata_two_groups) | ||
| calc.calculate_ratios('unique_counts') | ||
| ratios = calc.get_ratios_for_synt_id(1) |
Contributor
There was a problem hiding this comment.
question (testing): Add a test for get_ratios_for_synt_id when the requested Synt_id does not exist.
There’s no test for how get_ratios_for_synt_id behaves when the requested synt_id is missing from adata.var['Synt_id']. Please add a test that locks in the expected behaviour (e.g. raising a clear ValueError vs returning an empty array) so this edge case is covered against future regressions.
| result = filter_low_expressed_genes( | ||
| adata, min_expression=10, lib_size_normalization=None, verbose=False | ||
| ) | ||
| assert 't3' not in result.var_names |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add coverage for library_size_dependent combined with non-callable thresholds.
You already cover scalar, dict, and callable min_expression values, plus library_size_dependent=True for the callable case. Please also add a test for library_size_dependent=True with a non-callable threshold (scalar or dict), asserting either the expected behaviour or a clear error. This will document and fix the intended semantics of that parameter combination.
Suggested implementation:
class TestMinExpressionTypes:
def test_float_threshold(self, adata):
result = filter_low_expressed_genes(
adata, min_expression=10.0, lib_size_normalization=None, verbose=False
)
assert result.n_vars == 2
def test_scalar_threshold_with_library_size_dependent_raises(self, adata):
# Non-callable thresholds are not compatible with library_size_dependent=True
with pytest.raises(ValueError):
filter_low_expressed_genes(
adata,
min_expression=10.0,
lib_size_normalization=None,
library_size_dependent=True,
verbose=False,
)
def test_dict_threshold_with_library_size_dependent_raises(self, adata):
# Non-callable dict thresholds are not compatible with library_size_dependent=True
threshold = {"A": 10.0, "B": 5.0}
with pytest.raises(ValueError):
filter_low_expressed_genes(
adata,
min_expression=threshold,
lib_size_normalization=None,
library_size_dependent=True,
verbose=False,
)
def test_dict_threshold_per_sample(self, adata):
"""
Per-sample thresholds.
Group A sums: s1=300, s2=250, s3=250.- Ensure
pytestis imported at the top oftests/test_filter.pyif it is not already:import pytest
- Update the implementation of
filter_low_expressed_genes(where it lives) so that whenlibrary_size_dependent=Trueandmin_expressionis not callable (i.e. scalar or dict), it raises aValueErrormatching these tests’ expectations.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add tests for filtering and and allele_utils
Summary by Sourcery
Add comprehensive automated coverage for filtering utilities and allelic ratio calculations while fixing syntelog ratio lookup and preparing for tagged releases.
Bug Fixes:
Enhancements:
CI: