8000
Skip to content

Changes for releasing v1.33#3428

Merged
ewels merged 10 commits intomainfrom
v1.33
Dec 9, 2025
Merged

Changes for releasing v1.33#3428
ewels merged 10 commits intomainfrom
v1.33

Conversation

@ewels
Copy link
Copy Markdown
Member
@ewels ewels commented Dec 9, 2025

No description provided.

@claude
Copy link
Copy Markdown
claude bot commented Dec 9, 2025

PR Review Summary

This release PR looks solid overall. The changes primarily include version bumps, changelog updates, documentation additions, and several small bug fixes related to linting errors. The code quality is good and follows the project conventions.

Detailed Review

✅ Code Quality & Best Practices

Good practices observed:

  • motus.py (lines 157-250): Excellent refactoring of the bargraph plots. The changes properly separate data transformation from configuration, making the code cleaner and more maintainable. Moving the read_count_multiplier logic into the data preparation phase rather than the plot configuration is the right approach.
  • picard/IlluminaBasecallingMetrics.py (lines 105-118): Correct fix changing title to name keys in the plot configuration, which aligns with MultiQC's current API.
  • deeptools/plotFingerprint.py (line 177): Good defensive programming adding explicit float() conversions to ensure type consistency.
  • qc3C/qc3C.py (line 953): Proper type conversion wrapping the numpy calculation in float().

Documentation:

  • New module documentation for ribotish, seqkit, and sylphtax are well-structured and clear
  • Search patterns are properly documented for all modules
  • Changelog is comprehensive and well-organized

🔍 Minor Observations

Removed parameters:
Several instances of hide_zero_cats: False were removed from plot configurations in:

  • bamdst/bamdst.py (lines 453, 473)
  • dragen_fastqc/content_metrics.py (line 246)
  • humid/*.py (3 files)
  • mosdepth/mosdepth.py (line 316)
  • samtools/coverage.py (line 238)

Question: Was this parameter deprecated or is False now the default? This change is consistent across all modules, suggesting it's intentional cleanup, but it would be good to confirm this doesn't change any plot behavior unexpectedly.

🧪 Test Coverage

No new tests are included in this PR, which is appropriate for a release PR that primarily:

  • Bumps version numbers
  • Updates documentation
  • Fixes minor linting issues
  • Removes deprecated parameters

The substantive changes (motus refactoring, type conversions) are relatively low-risk and maintain existing behavior.

🔒 Security Concerns

No security issues identified. The changes are primarily:

  • Documentation updates
  • Configuration cleanups
  • Type conversions for data consistency

📊 Performance Considerations

Positive impact:

  • motus.py: The refactoring pre-computes multiplied values rather than using modify lambdas, which could provide a minor performance improvement for large datasets by avoiding repeated lambda calls during rendering.

No negative impact expected from other changes.

📝 Suggestions

  1. Version consistency: The PR bumps version from 1.33dev to 1.33 in pyproject.toml, which is correct for a release. After merging, remember to bump to 1.34dev for continued development.

  2. CHANGELOG.md: The changelog entry is well-structured. The duplicate entries that were moved from v1.32 to v1.33 section (glimpse decimal precision and axis_controlled_by_switches flag) have been correctly reorganized.

  3. Empty PR body: Consider adding a brief description to the PR body for future reference, even if it's just "Release preparation for v1.33".

✅ Overall Assessment

This is a clean release PR with:

  • ✅ Proper version bumping
  • ✅ Comprehensive changelog
  • ✅ Good code quality improvements
  • ✅ Consistent cleanup of deprecated parameters
  • ✅ No security concerns
  • ✅ Appropriate documentation updates

Recommendation: Approve and merge. This is ready for release.

@ewels ewels merged commit 5953b54 into main Dec 9, 2025
14 checks passed
@ewels ewels deleted the v1.33 branch December 9, 2025 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@ewels
0