-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BUG: fix CDS span calculations #196
Conversation
[CHANGED] reference the optional install groups to simplify the listings of dependencies [CHANGED] add support for python 3.13 and include in testing action
[NEW] used for conditional handling of adjusting translation boundaries
[CHANGED] relative start should not be adjusted for 0-based indexing [CHANGED] updated tests to this
Reviewer's Guide by SourceryThis pull request fixes CDS span calculations, adds tests for various CDS scenarios, and updates development dependencies and testing workflow to include Python 3.13 and resolve data download issues. The CDS span calculation fix ensures accurate sequence retrieval for both single and multi-exon transcripts. Sequence diagram for CDS span calculation with single exonsequenceDiagram
participant get_transcript_attr_records
participant LimitExons
participant cds_spans
get_transcript_attr_records->>LimitExons: lex.single_exon == True
alt lex.single_exon == True
get_transcript_attr_records->>cds_spans: ex_start = cds_spans[0][0] if lex.strand == 1 else cds_spans[0][1]
get_transcript_attr_records->>cds_spans: start, stop = ex_start + lex.rel_start, ex_start + lex.rel_stop (if lex.strand == 1)
get_transcript_attr_records->>cds_spans: start, stop = ex_start - lex.rel_stop, ex_start - lex.rel_start (if lex.strand != 1)
get_transcript_attr_records->>cds_spans: cds_spans[0, :] = start, stop
get_transcript_attr_records-->>get_transcript_attr_records: yield TranscriptAttrRecord
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @GavinHuttley - I've reviewed your changes - here's some feedback:
Overall Comments:
- The tests added in this PR are great, but could be improved by using smaller test fixtures rather than loading the entire genome.
- The logic for single exon CDS spans seems overly complex; can it be simplified?
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Pull Request Test Coverage Report for Build 13556843807Details
💛 - Coveralls |
Summary by Sourcery
Fixes CDS span calculations and adds tests to validate the fix. The pull request also updates the CI configuration to include Python 3.13 and updates the test data URL.
Bug Fixes:
Build:
requires-python
field inpyproject.toml
to allow Python 3.13.CI:
Tests: