v0.6.2 #3
v0.6.2 #3
Conversation
WalkthroughRelease 0.6.2 introduces compute configuration system for GPU/thread control in CLI, extends differential expression with group-wise FDR statistics, fixes layer inference null-check bug, significantly expands test coverage with new test modules and fixtures, optimizes test data sizes, and updates documentation with compute options. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Entry (main.py)
participant Parser as Parser
participant Env as Environment
participant Mellon as Mellon (import)
participant Compute as Compute Config
participant JAX as JAX Backend
participant NumPy as NumPy/Dask
participant DE as Differential Expression
CLI->>CLI: _set_early_thread_limits()
CLI->>Env: Set OMP/MKL/BLAS env vars
CLI->>Parser: Parse --threads, --use-gpu
Parser->>CLI: Return args
CLI->>Mellon: Import mellon (triggers JAX init)
Mellon->>JAX: JAX initialization
CLI->>Compute: configure_compute(use_gpu, n_threads)
Compute->>Env: _configure_thread_limits()
Env->>NumPy: Set thread limits
Compute->>JAX: _configure_jax(use_gpu, n_threads)
JAX->>JAX: Set XLA_FLAGS, platform
Compute->>NumPy: _configure_dask() (optional)
NumPy->>NumPy: Configure Dask scheduler
Compute->>Compute: log_compute_environment()
CLI->>DE: run_de(adata, params)
DE->>DE: compute_differential_expression()
DE->>DE: Generate group-wise FDR stats
DE->>DE: Store varm entries (local_fdr_groups, is_de_groups, ptp_groups)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
kompot/cli/main.py (1)
67-71: Version string mismatch.The version string shows
0.6.0but this PR is for releasev0.6.2. This should be updated to match the release version.parser.add_argument( '--version', action='version', - version='%(prog)s 0.6.0' + version='%(prog)s 0.6.2' )
🧹 Nitpick comments (23)
CHANGELOG.md (1)
5-11: LGTM! Consider fixing list indentation.The CHANGELOG entry accurately documents the v0.6.2 changes. However, the bullet points have inconsistent indentation.
Apply this diff to align with markdown style guidelines:
## [0.6.2] - - fix differential expression analysis using `groups` - - increase testing coverage - - thread and GPU-usage control in CLI - - fix `volcano_de` plot when the layer is `None` +- fix differential expression analysis using `groups` +- increase testing coverage +- thread and GPU-usage control in CLI +- fix `volcano_de` plot when the layer is `None`Based on static analysis hints from markdownlint-cli2.
pyproject.toml (1)
13-28: Pytest configuration and version bump look consistent; consider narrowing warning filtersThe updated pytest options, markers, and the 0.6.2 version bump are consistent with the rest of the PR. The broad
filterwarningsrules that ignore allDeprecationWarning/FutureWarningclasses will keep test output clean but may also hide important upstream deprecations; if you rely on these to catch API changes, consider narrowing the filters to specific modules/messages instead of silencing the entire categories.Also applies to: 40-40
kompot/anndata/differential_expression.py (1)
2616-2631: Minor: Redundant truncation logic for group ptp values.Lines 2621-2625 have overlapping conditions - after checking for
expanded_geneslength, theelifalso truncates for any length mismatch withselected_genes. While this is defensive, consider adding a debug log when hitting the fallback truncation to aid troubleshooting unexpected array sizes.if len(subset_ptp) == len(expanded_genes): subset_ptp = subset_ptp[:len(selected_genes)] elif len(subset_ptp) != len(selected_genes): + logger.debug( + f"Group '{subset_name}' ptp length {len(subset_ptp)} unexpected, truncating to {len(selected_genes)}" + ) # Truncate if too long subset_ptp = subset_ptp[:len(selected_genes)]tests/test_groupwise_fdr_integration.py (1)
386-392: Consider adding more specific consistency assertions.The test validates that DE counts are non-negative, but the comment notes this is a "soft check." Consider adding a more specific test that verifies at least some overlap between globally DE genes and group-wise DE genes when the signal is strong, to catch potential bugs where group results are completely independent of global results.
kompot/cli/da.py (1)
259-266: Consider catching more specific exceptions.The static analysis flagged the blind
Exceptioncatch (BLE001). While the graceful fallback is appropriate, this could inadvertently suppress critical errors. Consider catching more specific exceptions.try: # Import mellon to trigger its JAX configuration import mellon # Now configure our settings (will override mellon's CPU-only default if needed) configure_compute(use_gpu=use_gpu, n_threads=n_threads) - except Exception as e: + except (ImportError, RuntimeError, OSError) as e: logger.warning(f"Could not configure compute backend: {e}") logger.warning("Proceeding with default configuration")tests/test_runinfo_coverage.py (4)
122-124: Use raw string for regex pattern inmatch=.The pattern
"Run ID .* not found"contains regex metacharacters (.*). Use a raw string to make the intent explicit and prevent potential issues with escape sequences.def test_init_invalid_run_id(self, adata_with_de_history): """Test that invalid run_id raises error.""" - with pytest.raises(ValueError, match="Run ID .* not found"): + with pytest.raises(ValueError, match=r"Run ID .* not found"): RunInfo(adata_with_de_history, run_id=999)
294-297: Avoid equality comparison toTrue; use truthiness check.Per PEP 8, use
if summary['has_groups']:for boolean checks rather than== True.summary = run_info.get_summary() - assert summary['has_groups'] == True + assert summary['has_groups'] is True assert summary['groups_count'] == 5 assert 'groups' in summaryAlternatively, if only truthiness matters:
assert summary['has_groups']
451-456: Rename unused loop variablekeyto_key.The loop variable
keyis not used within the loop body.# Check that 'different' contains dicts with 'run1' and 'run2' keys - for key, value in param_comp['different'].items(): + for _key, value in param_comp['different'].items(): assert isinstance(value, dict) assert 'run1' in value assert 'run2' in value
489-496: Rename unused loop variablelocationto_location.The loop variable
locationis not used within the loop body.# Each location should have same, only_in_run1, only_in_run2 - for location, data in by_location.items(): + for _location, data in by_location.items(): assert 'same' in data assert 'only_in_run1' in data assert 'only_in_run2' in datakompot/cli/de.py (2)
276-284: Consider catching more specific exceptions or logging the exception type.While catching broad
Exceptionfor graceful degradation is reasonable here (as configuration failure shouldn't halt analysis), consider logging the exception class to aid debugging.try: # Import mellon to trigger its JAX configuration import mellon # Now configure our settings (will override mellon's CPU-only default if needed) configure_compute(use_gpu=use_gpu, n_threads=n_threads) except Exception as e: - logger.warning(f"Could not configure compute backend: {e}") + logger.warning(f"Could not configure compute backend ({type(e).__name__}): {e}") logger.warning("Proceeding with default configuration")
230-250: Redundant dictionary operations can be simplified.Several lines perform
args_dict['key'] = args_dict.pop('key')which is a no-op. These appear to be remnants from when CLI arg names differed from function parameter names.# Rename CLI args to match function parameters - if 'obsm_key' in args_dict: - args_dict['obsm_key'] = args_dict.pop('obsm_key') - if 'result_key' in args_dict: - args_dict['result_key'] = args_dict.pop('result_key') - if 'n_landmarks' in args_dict: - args_dict['n_landmarks'] = args_dict.pop('n_landmarks') - if 'sample_col' in args_dict: - args_dict['sample_col'] = args_dict.pop('sample_col') - if 'batch_size' in args_dict: - args_dict['batch_size'] = args_dict.pop('batch_size') - if 'fdr_threshold' in args_dict: - args_dict['fdr_threshold'] = args_dict.pop('fdr_threshold') - if 'null_genes' in args_dict: - args_dict['null_genes'] = args_dict.pop('null_genes') - if 'store_landmarks' in args_dict: - args_dict['store_landmarks'] = args_dict.pop('store_landmarks') - if 'store_additional_stats' in args_dict: - args_dict['store_additional_stats'] = args_dict.pop('store_additional_stats') + # Handle special CLI arg transformations if 'no_progress' in args_dict: args_dict['progress'] = not args_dict.pop('no_progress')The only meaningful transformation is
no_progress→progress. The other operations don't change anything.tests/test_field_tracking_coverage.py (2)
349-358: Prefix unused unpacked variable with underscore.The
prev_runvariable is unpacked but never used. Prefix with_to indicate intentional discard.- will_overwrite, overwritten, prev_run = detect_output_field_overwrite( + will_overwrite, overwritten, _prev_run = detect_output_field_overwrite( sample_adata, analysis_type="da", field_names=field_names, overwrite=False )This pattern applies to similar occurrences at lines 372, 394, 416, 431, 447, and 461.
443-456: Multiple unused unpacked variables in this test.Both
overwrittenandprev_runare unpacked but never used.- will_overwrite, overwritten, prev_run = detect_output_field_overwrite( + will_overwrite, _overwritten, _prev_run = detect_output_field_overwrite( sample_adata, result_type="differential_abundance", output_patterns=['field1'], overwrite=False, location="obs" )kompot/cli/compute_config.py (2)
23-54: UseOptional[int]for type hints withNonedefault.PEP 484 prohibits implicit
Optional. The parametern_threads: int = Noneshould ben_threads: Optional[int] = None.+from typing import Optional + -def configure_compute(use_gpu: bool = False, n_threads: int = None): +def configure_compute(use_gpu: bool = False, n_threads: Optional[int] = None):This also applies to
_configure_jax(line 108) and_configure_dask(line 169).
126-131: Remove extraneousfprefix from string without placeholders.if len(devices) > 0: - logger.info(f" JAX: GPU mode enabled") + logger.info(" JAX: GPU mode enabled") logger.info(f" Available GPU devices: {len(devices)}")tests/test_cli_dm_coverage.py (2)
280-306: Remove dead code in skipped test.This test contains unreachable code (lines 293-301) because it's always skipped at line 302. Either implement the test properly or remove the dead code to avoid confusion.
class TestDMRunOutputFormat: """Test run_dm output format handling.""" def test_run_dm_unsupported_output_format(self, sample_adata_with_pca, tmp_path): """Test run_dm with unsupported output format.""" - from kompot.cli.dm import run_dm - import argparse - - # Save sample data - input_file = tmp_path / 'input.h5ad' - sample_adata_with_pca.write_h5ad(input_file) - - # Mock palantir.utils.run_diffusion_maps - try: - import palantir - - def mock_run_dm(adata, **kwargs): - # Add DM_EigenVectors to adata - adata.obsm['DM_EigenVectors'] = np.random.randn(adata.n_obs, 10) - - import kompot.cli.dm - original_palantir = sys.modules.get('palantir') - - # Monkeypatch is tricky here, let's just test the parser instead - # This test would require more complex mocking - pytest.skip("Requires complex palantir mocking") - - except ImportError: - pytest.skip("Palantir not installed") + # TODO: Implement test for unsupported output format + # Requires complex palantir mocking + pytest.skip("Requires complex palantir mocking")
328-362: Unused variableargsin skipped test.The
argsvariable is assigned but never used because the test is always skipped. Either implement the test or simplify to just the skip statement.tests/test_cli_compute_config.py (2)
55-70: Improve exception handling in JAX configuration test.The broad
except Exception: passsilently swallows all errors, making debugging difficult. Consider catching specific exceptions or at least logging the error.def test_configure_jax_cpu(self, monkeypatch): """Test JAX configuration for CPU.""" from kompot.cli.compute_config import _configure_jax import jax _configure_jax(use_gpu=False, n_threads=None) # Check that JAX is configured for CPU # Note: This test may vary depending on JAX installation try: platform = jax.devices()[0].platform # Should be 'cpu' but might be different depending on environment assert platform in ['cpu', 'gpu'] - except Exception: - # If JAX is not properly configured, that's OK for this test - pass + except (RuntimeError, IndexError) as e: + # JAX may not be properly configured in all environments + pytest.skip(f"JAX not properly configured: {e}")
198-240: Remove unusedcapsysfixture.The
capsysfixture is declared but never used in this test method.- def test_run_de_missing_output(self, sample_adata_for_cli, tmp_path, capsys): + def test_run_de_missing_output(self, sample_adata_for_cli, tmp_path):tests/test_plot_expression_coverage.py (3)
130-136: Use raw string for regex pattern.The pattern contains
.which is a regex metacharacter. Use a raw string to make the intent clearer and avoid potential issues.def test_missing_gene_error(self): """Test that ValueError is raised for missing gene.""" - with pytest.raises(ValueError, match="not found in adata.var_names"): + with pytest.raises(ValueError, match=r"not found in adata\.var_names"): plot_gene_expression( self.adata, gene='NONEXISTENT_GENE' )
202-219: Consider usingpytest.importorskipor conditional assertions.The
try/except Exception: passpattern makes test failures silent. While acceptable for coverage tests with optional dependencies, consider usingpytest.importorskip('scanpy')or wrapping assertions in conditionals for clearer test behavior.
214-216: Prefix unused variable with underscore.The unpacked
axsvariable is unused. Use_axsor_to indicate it's intentionally unused.if result is not None: - fig, axs = result + fig, _axs = result assert fig is not None plt.close(fig)This pattern appears multiple times in the file (lines 232, 250, 268, 285, 303, 413, 444, 496, 520, 584, 600).
tests/test_heatmap_visualization_coverage.py (1)
72-75: Usecallable()instead ofhasattr(x, "__call__").
hasattr(x, "__call__")is unreliable for testing callability. Usecallable()for consistent results.# Should return a colormap object assert cmap_obj is not None - assert hasattr(cmap_obj, '__call__') # Colormap is callable + assert callable(cmap_obj) # Colormap is callable
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
.github/workflows/tests.yml(1 hunks)CHANGELOG.md(1 hunks)docs/source/cli.rst(2 hunks)kompot/anndata/differential_expression.py(5 hunks)kompot/cli/compute_config.py(1 hunks)kompot/cli/da.py(4 hunks)kompot/cli/de.py(4 hunks)kompot/cli/main.py(1 hunks)kompot/plot/expression.py(1 hunks)kompot/version.py(1 hunks)pyproject.toml(2 hunks)tests/conftest.py(1 hunks)tests/test_anndata_differential_abundance.py(1 hunks)tests/test_anndata_groups.py(2 hunks)tests/test_anndata_representation_check.py(8 hunks)tests/test_cleanup.py(1 hunks)tests/test_cleanup_multiple_runs.py(1 hunks)tests/test_cli_compute_config.py(1 hunks)tests/test_cli_dm_coverage.py(1 hunks)tests/test_de_advanced_features.py(1 hunks)tests/test_de_comprehensive_params.py(25 hunks)tests/test_de_error_handling.py(1 hunks)tests/test_direction_plot.py(1 hunks)tests/test_fdr_edge_cases.py(13 hunks)tests/test_fdr_integration.py(1 hunks)tests/test_field_tracking_coverage.py(1 hunks)tests/test_groupwise_fdr_integration.py(1 hunks)tests/test_heatmap_visualization_coverage.py(1 hunks)tests/test_html_representation.py(1 hunks)tests/test_plot_expression_coverage.py(1 hunks)tests/test_plot_functions.py(1 hunks)tests/test_posterior_covariance.py(1 hunks)tests/test_ptp_functionality.py(1 hunks)tests/test_runinfo_coverage.py(1 hunks)tests/test_single_condition_variance.py(5 hunks)tests/test_store_additional_stats.py(1 hunks)tests/test_volcano_de.py(1 hunks)tests/test_volcano_de_edge_cases.py(1 hunks)tests/test_volcano_de_fdr.py(1 hunks)tests/test_volcano_multi_da.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (14)
tests/test_field_tracking_coverage.py (1)
kompot/anndata/utils/field_tracking.py (8)
get_run_history(14-74)append_to_run_history(77-108)get_last_run_info(111-128)generate_output_field_names(131-272)get_environment_info(275-331)detect_output_field_overwrite(334-460)validate_field_run_id(482-538)get_run_from_history(541-668)
tests/test_plot_expression_coverage.py (1)
kompot/plot/expression.py (2)
plot_gene_expression(83-642)_infer_expression_keys(18-74)
tests/test_runinfo_coverage.py (1)
kompot/anndata/utils/runinfo.py (6)
RunInfo(16-852)RunComparison(855-1593)_get_fields_for_run(121-185)get_raw_data(440-449)get_data(346-392)compare_with(330-344)
tests/test_volcano_multi_da.py (8)
tests/test_anndata_groups.py (1)
create_test_anndata(44-105)tests/test_anndata_differential_abundance.py (1)
create_test_anndata(15-50)tests/test_direction_plot.py (1)
create_test_anndata(17-48)tests/test_html_representation.py (1)
create_test_anndata(11-37)tests/test_plot_functions.py (1)
create_test_anndata(18-50)tests/test_posterior_covariance.py (1)
create_test_anndata(12-61)tests/test_volcano_de.py (1)
create_test_anndata(17-91)tests/test_anndata_functions.py (1)
create_test_anndata(13-59)
tests/test_html_representation.py (8)
tests/test_anndata_groups.py (1)
create_test_anndata(44-105)tests/test_anndata_differential_abundance.py (1)
create_test_anndata(15-50)tests/test_direction_plot.py (1)
create_test_anndata(17-48)tests/test_plot_functions.py (1)
create_test_anndata(18-50)tests/test_posterior_covariance.py (1)
create_test_anndata(12-61)tests/test_volcano_de.py (1)
create_test_anndata(17-91)tests/test_volcano_multi_da.py (1)
create_test_anndata(17-48)tests/test_anndata_functions.py (1)
create_test_anndata(13-59)
tests/test_plot_functions.py (8)
tests/test_anndata_groups.py (1)
create_test_anndata(44-105)tests/test_anndata_differential_abundance.py (1)
create_test_anndata(15-50)tests/test_direction_plot.py (1)
create_test_anndata(17-48)tests/test_html_representation.py (1)
create_test_anndata(11-37)tests/test_posterior_covariance.py (1)
create_test_anndata(12-61)tests/test_volcano_de.py (1)
create_test_anndata(17-91)tests/test_volcano_multi_da.py (1)
create_test_anndata(17-48)tests/test_anndata_functions.py (1)
create_test_anndata(13-59)
tests/test_heatmap_visualization_coverage.py (1)
kompot/plot/heatmap/visualization.py (3)
_draw_diagonal_split_cell(72-193)_draw_split_dot_cell(196-378)_draw_fold_change_cell(380-458)
kompot/cli/da.py (1)
kompot/cli/compute_config.py (1)
configure_compute(23-75)
kompot/cli/de.py (1)
kompot/cli/compute_config.py (1)
configure_compute(23-75)
kompot/anndata/differential_expression.py (1)
kompot/anndata/fdr_utils.py (1)
compute_fdr_statistics(124-219)
tests/test_groupwise_fdr_integration.py (1)
kompot/anndata/differential_expression.py (1)
compute_differential_expression(277-2858)
tests/test_de_error_handling.py (1)
tests/test_group_utils_comprehensive.py (1)
create_test_adata(15-39)
tests/test_anndata_differential_abundance.py (8)
tests/test_anndata_groups.py (1)
create_test_anndata(44-105)tests/test_direction_plot.py (1)
create_test_anndata(17-48)tests/test_html_representation.py (1)
create_test_anndata(11-37)tests/test_plot_functions.py (1)
create_test_anndata(18-50)tests/test_posterior_covariance.py (1)
create_test_anndata(12-61)tests/test_volcano_de.py (1)
create_test_anndata(17-91)tests/test_volcano_multi_da.py (1)
create_test_anndata(17-48)tests/test_anndata_functions.py (1)
create_test_anndata(13-59)
kompot/cli/main.py (1)
kompot/cli/utils.py (1)
setup_logging(98-112)
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
7-7: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
8-8: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
9-9: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
10-10: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
🪛 Ruff (0.14.6)
tests/test_field_tracking_coverage.py
372-372: Unpacked variable prev_run is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
394-394: Unpacked variable prev_run is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
416-416: Unpacked variable prev_run is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
431-431: Unpacked variable prev_run is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
447-447: Unpacked variable overwritten is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
447-447: Unpacked variable prev_run is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
461-461: Unpacked variable prev_run is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
tests/test_plot_expression_coverage.py
88-88: Do not assert blind exception: Exception
(B017)
132-132: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
214-214: Unpacked variable axs is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
217-219: try-except-pass detected, consider logging the exception
(S110)
217-217: Do not catch blind exception: Exception
(BLE001)
232-232: Unpacked variable axs is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
235-237: try-except-pass detected, consider logging the exception
(S110)
235-235: Do not catch blind exception: Exception
(BLE001)
250-250: Unpacked variable axs is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
253-254: try-except-pass detected, consider logging the exception
(S110)
253-253: Do not catch blind exception: Exception
(BLE001)
268-268: Unpacked variable axs is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
271-272: try-except-pass detected, consider logging the exception
(S110)
271-271: Do not catch blind exception: Exception
(BLE001)
285-285: Unpacked variable axs is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
288-289: try-except-pass detected, consider logging the exception
(S110)
288-288: Do not catch blind exception: Exception
(BLE001)
303-303: Unpacked variable axs is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
306-307: try-except-pass detected, consider logging the exception
(S110)
306-306: Do not catch blind exception: Exception
(BLE001)
324-326: try-except-pass detected, consider logging the exception
(S110)
324-324: Do not catch blind exception: Exception
(BLE001)
344-345: try-except-pass detected, consider logging the exception
(S110)
344-344: Do not catch blind exception: Exception
(BLE001)
359-360: try-except-pass detected, consider logging the exception
(S110)
359-359: Do not catch blind exception: Exception
(BLE001)
413-413: Unpacked variable axs is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
415-416: try-except-pass detected, consider logging the exception
(S110)
415-415: Do not catch blind exception: Exception
(BLE001)
444-444: Unpacked variable axs is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
446-447: try-except-pass detected, consider logging the exception
(S110)
446-446: Do not catch blind exception: Exception
(BLE001)
496-496: Unpacked variable axs is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
498-499: try-except-pass detected, consider logging the exception
(S110)
498-498: Do not catch blind exception: Exception
(BLE001)
520-520: Unpacked variable axs is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
522-523: try-except-pass detected, consider logging the exception
(S110)
522-522: Do not catch blind exception: Exception
(BLE001)
584-584: Unpacked variable axs is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
586-587: try-except-pass detected, consider logging the exception
(S110)
586-586: Do not catch blind exception: Exception
(BLE001)
600-600: Unpacked variable axs is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
602-603: try-except-pass detected, consider logging the exception
(S110)
602-602: Do not catch blind exception: Exception
(BLE001)
kompot/cli/compute_config.py
23-23: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
108-108: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
128-128: f-string without any placeholders
Remove extraneous f prefix
(F541)
169-169: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
222-222: Do not catch blind exception: Exception
(BLE001)
tests/test_runinfo_coverage.py
123-123: Pattern passed to match= contains metacharacters but is neither escaped nor raw
(RUF043)
295-295: Avoid equality comparisons to True; use summary['has_groups']: for truth checks
Replace with summary['has_groups']
(E712)
452-452: Loop control variable key not used within loop body
Rename unused key to _key
(B007)
490-490: Loop control variable location not used within loop body
Rename unused location to _location
(B007)
tests/test_heatmap_visualization_coverage.py
23-23: Unpacked variable cmap_obj is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
36-36: Unpacked variable cmap_obj is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
47-47: Unpacked variable norm is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
47-47: Unpacked variable cmap_obj is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
57-57: Unpacked variable norm is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
57-57: Unpacked variable cmap_obj is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
68-68: Unpacked variable norm is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
68-68: Unpacked variable vmin is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
68-68: Unpacked variable vmax is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
74-74: Using hasattr(x, "__call__") to test if x is callable is unreliable. Use callable(x) for consistent results.
Replace with callable()
(B004)
80-80: Unpacked variable norm is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
80-80: Unpacked variable vmin is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
80-80: Unpacked variable vmax is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
89-89: Unpacked variable norm is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
89-89: Unpacked variable cmap_obj is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
tests/test_cli_compute_config.py
55-55: Unused method argument: monkeypatch
(ARG002)
68-70: try-except-pass detected, consider logging the exception
(S110)
68-68: Do not catch blind exception: Exception
(BLE001)
85-85: Unused method argument: monkeypatch
(ARG002)
167-167: Unused method argument: monkeypatch
(ARG002)
198-198: Unused method argument: capsys
(ARG002)
424-424: Unused function argument: adata
(ARG001)
424-424: Unused function argument: kwargs
(ARG001)
484-484: Unused function argument: adata
(ARG001)
484-484: Unused function argument: kwargs
(ARG001)
592-592: Unused function argument: adata
(ARG001)
592-592: Unused function argument: kwargs
(ARG001)
841-841: Unused function argument: adata
(ARG001)
841-841: Unused function argument: kwargs
(ARG001)
899-899: Unused function argument: adata
(ARG001)
899-899: Unused function argument: kwargs
(ARG001)
1003-1003: Unused function argument: adata
(ARG001)
1003-1003: Unused function argument: kwargs
(ARG001)
tests/test_cli_dm_coverage.py
164-164: Avoid specifying long messages outside the exception class
(TRY003)
293-293: Unused function argument: kwargs
(ARG001)
298-298: Local variable original_palantir is assigned to but never used
Remove assignment to unused variable original_palantir
(F841)
329-329: Local variable args is assigned to but never used
Remove assignment to unused variable args
(F841)
kompot/cli/da.py
264-264: Do not catch blind exception: Exception
(BLE001)
kompot/cli/de.py
281-281: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test (3.9)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
- GitHub Check: test (3.11)
- GitHub Check: test (3.9)
🔇 Additional comments (61)
.github/workflows/tests.yml (1)
5-5: LGTM!The addition of the
devbranch to both push and pull_request triggers is appropriate and aligns with the release workflow (dev→main merge). The workflow configuration is sound: pinned action versions, correct JAX CPU setup to avoid CI GPU dependencies, and proper pytest/coverage integration.Also applies to: 7-7
tests/test_cleanup_multiple_runs.py (1)
8-8: Reduced defaultn_cellsto 60 looks goodLowering the default from 100 to 60 keeps the data shape and test logic intact (no assertions depend on absolute cell counts) while reducing test runtime and memory use. This is consistent with the broader goal of shrinking test datasets across the suite.
tests/test_fdr_integration.py (1)
10-10: Test optimization looks reasonable with appropriate compensations.The reduction from 100 to 60 cells (30 per condition) decreases test runtime while maintaining test validity through stronger effect sizes (5-10x fold changes on line 31). For integration tests, this trade-off is acceptable.
However, note that real-world edge cases with smaller effect sizes or borderline statistical significance may not be well-represented in these tests.
tests/test_direction_plot.py (1)
17-17: LGTM! Test data size optimization.Reducing the default test data size from 100 to 60 cells improves test performance while maintaining adequate coverage.
tests/test_plot_functions.py (1)
18-18: LGTM! Test data size optimization.Consistent with the test suite optimization to reduce default data sizes for faster test execution.
kompot/version.py (1)
3-3: LGTM! Version bump for release.Version correctly updated to 0.6.2, consistent with the PR objectives and CHANGELOG.
tests/test_ptp_functionality.py (1)
15-15: LGTM! Test data size optimization.Reducing default cells from 100 to 60 improves test performance, consistent with the broader test suite optimization.
tests/test_posterior_covariance.py (1)
12-12: LGTM! Test data size optimization.Reducing default cells from 100 to 60 aligns with the test suite-wide performance optimization.
tests/test_fdr_edge_cases.py (1)
9-9: LGTM! Test data size optimization.Default test data size reduced from 100 to 60 cells, consistent with the performance improvements across the test suite.
tests/test_volcano_multi_da.py (1)
17-17: LGTM! Test data size optimization.Default test cells reduced from 100 to 60, improving test performance while maintaining coverage.
docs/source/cli.rst (1)
248-255: Compute options docs align with new CLI behaviorThe new “Compute Options” sections for
--use-gpuand--threadsunder both DE and DA commands are clear and match the compute-configuration features added in this release. No issues from a docs/UX perspective.Also applies to: 336-343
kompot/plot/expression.py (1)
223-230: Robust None-handling in inferred layer selectionAdding the
inferred_layer is not Noneguard before checking for"fold_change"avoids a potentialTypeErrorwhen the stored layer isNone, while still excluding fold-change layers from expression visualization and logging the fallback correctly. This is a safe and behavior-preserving fix.tests/test_cleanup.py (1)
8-40: Reduced test fixture size is safe and improves test performanceChanging
create_test_adata_for_cleanupto use 60 cells by default keeps all test logic intact (splits and landmarks are still well within bounds) while reducing computation and memory in the cleanup tests.tests/test_anndata_representation_check.py (1)
12-67: Underrepresentation test fixture shrink preserves intended imbalanceUpdating the helper to
n_cells=60and adjusting its usages keeps the extreme imbalance between conditions within each tissue (e.g. ~95/5 splits per tissue) while reducing test cost. The expectations in the tests (which assert specific tissues/conditions are underrepresented) remain consistent with the new counts.Also applies to: 71-71, 170-173, 225-229, 276-280, 333-336, 401-405, 440-446
tests/test_volcano_de.py (1)
17-58: Smaller volcano test AnnData fixture keeps behavior but speeds testsReducing the default
n_cellsto 60 while still splitting evenly between groups A and B maintains the structure required by the volcano plot tests and improves test runtime/memory.tests/test_de_error_handling.py (1)
10-25: DE error-handling tests remain valid with smaller default n_cellsThe updated
create_test_adatadefault of 60 cells still yields balanced A/B conditions and consistent obsm shapes, while specific tests that exercise edge sizes overriden_cellsexplicitly. Error-handling coverage is preserved with lower resource usage.tests/test_single_condition_variance.py (2)
19-37: Sample layouts correctly exercise single-/multi-sample variance scenariosThe updated 60-cell setup and revised
samples_*assignments still match the intended scenarios:
- base
samplecolumn: cond1 has two samples, cond2 one sample;sample_both: both conditions have multiple samples;sample_fallback/sample_reverse: only one condition has multiple samples, triggering the fallback reuse of a single-condition variance;sample_both_fail: both conditions single-sample, ensuring the expected error path.This keeps the variance-handling edge cases well covered with a smaller fixture.
Also applies to: 29-31, 105-110, 163-169, 188-190, 209-213
154-161: Relaxed tolerance for comparing DE results without sample_col is reasonableRelaxing
rtolto 0.05 when comparingmahalanobisvalues betweenallow_single_condition_variance=FalseandTrue(withsample_col=None) is appropriate given the reduced dataset size and any minor numerical variability, while still asserting that both modes produce effectively equivalent results in the no-sample-col case.tests/test_de_comprehensive_params.py (1)
12-16: Test data size optimization looks appropriate.The reduction from 120 to 50 cells with
n_landmarks=10is a reasonable trade-off for faster test execution while still maintaining adequate coverage for parameter combination testing.kompot/anndata/differential_expression.py (5)
2368-2376: Group-wise FDR varm key initialization looks correct.The conditional addition of FDR-related varm keys (
local_fdr_groups,is_de_groups,ptp_groups) is properly gated byuse_fdr,null_gene_indices, andcompute_mahalanobisflags, consistent with the global FDR logic.
2494-2510: Correct handling of null genes in subset mean_log_fold_change.The logic properly detects when subset results include null genes (by checking against
expanded_geneslength) and extracts only the real genes. The fallback warning for unexpected lengths is appropriate.
2544-2560: Consistent null gene handling for mahalanobis_distances.Same correct pattern as
mean_log_fold_change- extracts real genes when the array includes null genes from FDR computation.
2574-2614: Group-wise FDR computation implementation looks solid.The implementation correctly:
- Guards computation with appropriate conditions (
use_fdr,null_gene_indices,compute_mahalanobis)- Validates array length before splitting real/null genes
- Reuses
compute_fdr_statisticsfor consistency with global FDR- Logs informative per-group DE summaries
2655-2685: Field mapping for group-wise FDR is comprehensive.The field mappings correctly document all new group-wise varm keys with appropriate metadata including location, type, description, and the list of subsets contained in each matrix.
tests/test_groupwise_fdr_integration.py (3)
18-48: Well-structured test fixture with appropriate data setup.The fixture creates test data with:
- Clear differential expression signal (first 10 genes shifted by +3.0 in condition2)
- Three balanced groups across two conditions
- Appropriate sizes (120 cells, 80 genes) for meaningful FDR testing
119-143: Good validation of boolean dtype for is_de.Testing that
is_devalues maintain boolean dtype is important for downstream filtering operations. This catches potential issues where the values might become floats or objects.
230-255: Excellent regression test for length mismatch warnings.Capturing log output to verify no length-mismatch warnings are emitted is a valuable regression test that will catch issues if the null gene handling logic breaks in the future.
tests/test_de_advanced_features.py (4)
21-39: Good test data setup with appropriate sample structure.The setup creates balanced conditions with multiple samples per condition, which is necessary for testing sample variance features.
41-73: Thorough FDR edge case testing.The test correctly creates extreme expression differences to trigger zero p-values, then validates that the FDR computation handles this gracefully with valid probability outputs.
154-196: Good pattern for handling optional feature compatibility.The try/except with
pytest.skipfor the progress parameter is a clean way to handle version differences in the underlyingmellonlibrary without failing the test suite.
245-268: Perfect! I have completed the verification. Let me now generate the final rewritten review comment.
Dict-based groups format is correct and matches parse_groups implementation.
The
groups_dictstructure with inner dicts using{'cell_type': 'TypeX'}format is the correct "named filters" pattern. Theparse_groupsfunction explicitly handles dict-of-dicts input where outer keys are filter names and inner dicts specify column conditions. The test setup properly creates acell_typecolumn inadata.obswith matching values (TypeX,TypeY), so the format will work as intended.tests/test_volcano_de_edge_cases.py (1)
17-17: Test data size optimization consistent with other test files.The reduction to 60 cells aligns with similar optimizations in other volcano test files (
test_volcano_de.py,test_volcano_de_fdr.py, etc.) as noted in the AI summary.tests/test_store_additional_stats.py (1)
8-8: LGTM - consistent with test suite optimization.The reduction of default
n_cellsfrom 100 to 60 aligns with the broader PR pattern of reducing test data sizes across the test suite for faster execution while maintaining test coverage.tests/test_html_representation.py (1)
11-11: LGTM - consistent test data sizing.The default
n_cells=60matches the pattern across other test modules (test_plot_functions.py,test_direction_plot.py,test_volcano_multi_da.py, etc.) for streamlined test execution.tests/test_anndata_differential_abundance.py (1)
15-15: LGTM - cleaner sample division.The change to
n_cells=60actually improves thewith_samples=Truecase: 60 cells divide evenly by 6 (3 samples × 2 conditions), giving exactly 10 cells per sample without remainders.kompot/cli/da.py (2)
151-162: LGTM - CLI compute configuration options.The
--use-gpuand--threadsarguments are well-documented and follow the existing CLI argument patterns. The help text clearly explains their purpose.
196-209: LGTM - early logging of compute configuration.Logging the GPU and thread configuration before setup provides good visibility into the intended compute environment.
tests/test_anndata_groups.py (2)
33-35: LGTM - necessary filtering for new FDR keys.The addition of
"fdr" not in keycorrectly prevents false matches with the new group-wise FDR statistics keys (e.g.,mahalanobis_local_fdr). The comment on line 34 clearly explains the intent.
44-44: LGTM - consistent test data sizing.Aligns with the PR-wide pattern of reducing default test cell counts to 60.
kompot/cli/main.py (2)
12-48: LGTM - early thread limit configuration.The pre-argparse parsing of
--threadsis a clever solution for setting environment variables before NumPy initialization. The function correctly handles both--threads VALUEand--threads=VALUEformats, and appropriately defers invalid value handling to the full argparse parsing.
53-59: LGTM - correct import deferral pattern.Moving subcommand imports after
_set_early_thread_limits()ensures NumPy (imported by those modules) respects the thread limits.tests/test_runinfo_coverage.py (2)
16-46: Well-structured test fixture with realistic data.The fixture properly sets up AnnData with differential expression history, using appropriate random seeds for reproducibility and reasonable test data sizes.
566-603: Good edge case coverage for RunInfo and RunComparison.The edge case tests effectively cover scenarios like JSON string field mappings, missing field mappings, and comparing a run with itself. This helps ensure robustness.
tests/test_volcano_de_fdr.py (1)
14-16: Reasonable test data size reduction.Reducing
n_cellsfrom 100 to 60 aligns with the PR objective to optimize test data sizes for improved test runtime while maintaining adequate coverage.kompot/cli/de.py (1)
164-175: New compute configuration CLI options look good.The
--use-gpuand--threadsoptions are well-documented with appropriate help text and correctly useaction='store_true'for the boolean flag andtype=intfor threads.tests/test_field_tracking_coverage.py (2)
168-302: Comprehensive field name generation tests.Excellent coverage of
generate_output_field_namesincluding DA/DE types, sample suffix handling, sanitization, and error cases. This will help catch regressions in field naming logic.
616-741: Thorough edge case testing forget_run_from_history.Good coverage of empty history, None run_id, positive/negative indices, out-of-range indices, JSON string handling, invalid JSON, non-dict items, custom history keys, and immutability verification.
kompot/cli/compute_config.py (4)
219-223: Broad exception catch is acceptable here for robustness.Catching
Exceptionwhen querying device platform is reasonable since this is informational and shouldn't fail the program. The fallback to'unknown'is appropriate.
78-106: Thread limit configuration is well-structured.Setting OMP, MKL, OpenBLAS, and BLAS thread limits covers the major NumPy backends. Good logging at both info and debug levels.
195-237: Clean implementation ofget_device_info().Handles missing JAX gracefully, checks for GPU availability, and returns a well-structured dictionary. Good defensive coding.
148-166: XLA_FLAGS formatting is correct per official documentation.The current implementation properly follows the documented XLA_FLAGS format. Official JAX documentation specifies that XLA_FLAGS should be a space-separated string with
--prefixed flags and non-prefixed parameters (e.g.,--xla_cpu_multi_thread_eigen=false intra_op_parallelism_threads=1). The code correctly combines both flags in a single string without requiring separate formatting or additional--prefixes. No changes needed.tests/test_cli_dm_coverage.py (2)
18-35: Consider using shared fixtures from conftest.py.This fixture duplicates functionality already provided by
small_adataintests/conftest.py. Using shared fixtures would reduce code duplication and ensure consistent test data across the test suite.However, this fixture includes PCA data (
X_pca) which is specifically required for DM tests, so keeping it as a specialized fixture is reasonable.
368-425: Good test structure with proper mocking.The success path tests properly mock
palantir.utils.run_diffusion_mapsand verify output file creation and content. The graceful handling of missing palantir dependency viapytest.skipis appropriate.tests/test_cli_compute_config.py (2)
371-382: Mock function signatures are appropriate.The unused
adataandkwargsparameters in mock functions (e.g.,mock_compute_de) are necessary to match the signature of the realcompute_differential_expressionfunction. The static analysis warnings for ARG001 are false positives in this context.
1-16: Good test module structure.Comprehensive coverage for CLI compute configuration including thread limits, JAX/Dask configuration, device info, and DE/DA CLI flows. The test organization is clear and follows pytest conventions.
tests/conftest.py (3)
9-11: Good practice: JAX environment configured before imports.Setting
JAX_PLATFORMSandJAX_ENABLE_X64before JAX is imported ensures consistent CPU-only testing behavior. This is the correct approach for test environments.
31-68: Well-designed tiered fixture system.The fixtures provide appropriately sized test data for different test scenarios:
tiny_adata: Fast unit tests (~0.01s)small_adata: Standard unit tests (~0.05s)medium_adata: Integration tests (~0.2-0.5s)This allows tests to choose the appropriate fixture based on their needs, balancing test coverage with execution speed.
181-212: Useful parameter fixtures for fast testing.The
fast_de_paramsandfast_da_paramsfixtures provide sensible defaults for quick test execution (smalln_landmarks, disabled FDR, no progress bars). This is good for CI/CD pipelines where test speed matters.tests/test_heatmap_visualization_coverage.py (3)
101-107: Good resource management with setup/teardown.Properly creating figures in
setup_methodand closing them inteardown_methodprevents matplotlib figure accumulation and memory leaks during test runs.
197-216: Comprehensive error handling tests.Good coverage testing invalid axes input and various edge cases (NaN values, string conversion, colormap objects). The tests verify both successful paths and error conditions.
402-413: Good edge case: counts exceeding global_max_count.Testing the clipping behavior when cell counts exceed
global_max_countensures the visualization function handles boundary conditions correctly.
| def test_infer_keys_strict_mode(self): | ||
| """Test that strict mode raises error when keys can't be inferred.""" | ||
| # AnnData without DE results | ||
| with pytest.raises(Exception): # Should raise when keys can't be inferred | ||
| _infer_expression_keys( | ||
| self.adata, | ||
| lfc_key=None, | ||
| score_key=None, | ||
| strict=True | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the _infer_expression_keys function implementation
find . -type f -name "*.py" | xargs grep -l "_infer_expression_keys" | head -20Repository: settylab/kompot
Length of output: 190
🏁 Script executed:
#!/bin/bash
# Find and examine the _infer_expression_keys function implementation
rg -n "_infer_expression_keys" kompot/plot/expression.py -A 30 -B 2Repository: settylab/kompot
Length of output: 2953
🏁 Script executed:
#!/bin/bash
# Get the full _infer_expression_keys function - read more lines
sed -n '18,100p' kompot/plot/expression.pyRepository: settylab/kompot
Length of output: 2760
🏁 Script executed:
#!/bin/bash
# Find the field_inference module and the infer_fields_from_run_info function
find . -type f -name "field_inference.py"Repository: settylab/kompot
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Find the infer_fields_from_run_info function and examine its error handling
rg -n "def infer_fields_from_run_info" ./kompot/plot/field_inference.py -A 50Repository: settylab/kompot
Length of output: 1943
🏁 Script executed:
#!/bin/bash
# Look for raise statements in the infer_fields_from_run_info function to confirm the exception type
rg -n "raise" ./kompot/plot/field_inference.py -B 2 -A 1Repository: settylab/kompot
Length of output: 687
🏁 Script executed:
#!/bin/bash
# Examine the test to see what it's currently testing
sed -n '85,94p' tests/test_plot_expression_coverage.pyRepository: settylab/kompot
Length of output: 467
Use ValueError instead of broad Exception for strict mode error handling.
The _infer_expression_keys function calls infer_fields_from_run_info with strict=True, which raises ValueError (not KeyError) when required fields cannot be inferred. Using pytest.raises(Exception) is too broad and may mask unintended errors.
def test_infer_keys_strict_mode(self):
"""Test that strict mode raises error when keys can't be inferred."""
# AnnData without DE results
- with pytest.raises(Exception): # Should raise when keys can't be inferred
+ with pytest.raises(ValueError): # Should raise when keys can't be inferred
_infer_expression_keys(
self.adata,
lfc_key=None,
score_key=None,
strict=True
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_infer_keys_strict_mode(self): | |
| """Test that strict mode raises error when keys can't be inferred.""" | |
| # AnnData without DE results | |
| with pytest.raises(Exception): # Should raise when keys can't be inferred | |
| _infer_expression_keys( | |
| self.adata, | |
| lfc_key=None, | |
| score_key=None, | |
| strict=True | |
| ) | |
| def test_infer_keys_strict_mode(self): | |
| """Test that strict mode raises error when keys can't be inferred.""" | |
| # AnnData without DE results | |
| with pytest.raises(ValueError): # Should raise when keys can't be inferred | |
| _infer_expression_keys( | |
| self.adata, | |
| lfc_key=None, | |
| score_key=None, | |
| strict=True | |
| ) |
🧰 Tools
🪛 Ruff (0.14.6)
88-88: Do not assert blind exception: Exception
(B017)
🤖 Prompt for AI Agents
In tests/test_plot_expression_coverage.py around lines 85 to 94, the test
currently expects a broad Exception when keys can't be inferred; replace
pytest.raises(Exception) with pytest.raises(ValueError) so the test specifically
asserts the ValueError raised by strict mode in
_infer_expression_keys/infer_fields_from_run_info, keeping the rest of the call
unchanged.
Update the main branch for release v0.6.2:
groupsvolcano_deplot when the layer isNoneSummary by CodeRabbit
Release Notes v0.6.2
New Features
--use-gpuflag for CLI commands.--threadsflag to limit compute threads for JAX, NumPy, and Dask.Bug Fixes
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.