Add a method for progress logging multiple alignments #199
Conversation
clintval
force-pushed
the
cv_template_logger
branch
from
0519fdf to
8b854cd
Compare
clintval
force-pushed
the
cv_template_logger
branch
from
ffd2071 to
c5791a1
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #199 +/- ##
==========================================
+ Coverage 91.31% 91.34% +0.02%
==========================================
Files 18 18
Lines 2281 2287 +6
Branches 334 335 +1
==========================================
+ Hits 2083 2089 +6
Misses 126 126
Partials 72 72 ☔ View full report in Codecov by Sentry. |
|
Warning Rate limit exceeded@clintval has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 53 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe pull request introduces a new method A corresponding test function, Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/fgpyo/util/test_logging.py (1)
88-101: Verifying logging functionality
The test ensures the logger captures all records. Consider adding a scenario where a template has zero records to confirm a no-op.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fgpyo/util/logging.py(2 hunks)tests/fgpyo/util/test_logging.py(2 hunks)
🔇 Additional comments (5)
tests/fgpyo/util/test_logging.py (3)
7-7: Import is correct
Good job importing Template to test the new functionality.
65-71: Test creation is clear
The test covers mapped, secondary, and supplementary reads. This comprehensive setup ensures thorough testing of complex templates.
77-86: Template construction approach
The manual construction of the expected Template with secondaries and supplementaries is appropriate. No issues found.
fgpyo/util/logging.py (2)
47-47: Importing Template
Needed to reference Template within record_template.
168-184: Method implementation
The method iterates all records in the provided Template and logs each. The logic is straightforward and consistent with record_alignment usage.
|
I don't object to this, but I wonder if a slightly different approach would work for you and be more general: def record_alignments(
self,
alns: Iterable[AlignedSegment],
) -> bool:
"""Correctly record all records in an iterable of alignments.
Args:
alns: an iterable of zero or more alignments
Returns:
true if a message was logged, false otherwise
"""
logged_message: bool = False
for rec in alns:
logged_message = self.record_alignment(rec) or logged_message
return logged_messageThat could then be called with |
|
@tfenne that also works. If I went that approach I might suggest making a small API break (in the naming of the parameter only) since we already have a diff --git a/fgpyo/util/logging.py b/fgpyo/util/logging.py
--- a/fgpyo/util/logging.py
+++ b/fgpyo/util/logging.py
@@ -150,20 +150,23 @@ class ProgressLogger(AbstractContextManager):
def record_alignment(
self,
- rec: AlignedSegment,
+ *recs: AlignedSegment,
) -> bool:
"""Correctly record pysam.AlignedSegments (zero-based coordinates).Then usage would be like: progress.record_alignment(*template.all_recs())I often have both iterables of alignments to log and templates so I wouldn't mind having both upgrades (variadic support for Let me know which option you like the most and I can fixup. |
I'm not sure I like the variadic arg version without changing the name (e.g. to But I'd be fine to add a |
clintval
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
fgpyo/util/logging.py (1)
171-178: Add usage example to docstring.Add example showing how to use with BAM records.
"""Correctly record multiple pysam.AlignedSegments (zero-based coordinates). Args: recs: pysam.AlignedSegment objects Returns: true if a message was logged, false otherwise + +Example: + >>> from pysam import AlignmentFile + >>> bam = AlignmentFile("input.bam") + >>> progress.record_alignments(bam.fetch()) """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
fgpyo/util/logging.py(2 hunks)tests/fgpyo/util/test_logging.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/fgpyo/util/test_logging.py
🔇 Additional comments (2)
fgpyo/util/logging.py (2)
41-41: LGTM!
Import correctly placed with other typing imports.
167-182: Implementation looks solid.
Correctly handles multiple alignments while maintaining logging behavior.
|
@tfenne I thought about this more and chose to ditch the idea of recording "templates". Simply because I didn't want a user to think the logger was incrementing |
clintval
force-pushed
the
cv_template_logger
branch
from
3b12692 to
c93fc80
Compare
Often I am iterating through a BAM by query name and find I'm having to manually loop over all records to increment the progress logger. This new method would make logging all the records in a template more convenient.