Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: use pybwa #113

Merged
merged 3 commits into from
Jan 22, 2025
Merged

feat: use pybwa #113

merged 3 commits into from
Jan 22, 2025

Conversation

nh13
Copy link
Member

@nh13 nh13 commented Jan 17, 2025 •
edited
Loading

No description provided.

@nh13 nh13 force-pushed the feat/pybwa branch 2 times, most recently from 19215f0 to 5f51557 Compare January 21, 2025 19:31
@codecov Codecov
Copy link

codecov bot commented Jan 21, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (dev@8e1de20). Learn more about missing BASE report.
Report is 2 commits behind head on dev.

Additional details and impacted files 
@@          Coverage Diff           @@
##             dev     #113   +/-   ##
======================================
  Coverage       ?   96.31%           
======================================
  Files          ?       18           
  Lines          ?     1546           
  Branches       ?      179           
======================================
  Hits           ?     1489           
  Misses         ?       38           
  Partials       ?       19

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nh13 nh13 force-pushed the feat/pybwa branch 2 times, most recently from 314c62c to 81ec0cf Compare January 22, 2025 00:15
@nh13 nh13 marked this pull request as ready for review January 22, 2025 00:19
@nh13 nh13 requested a review from tfenne as a code owner January 22, 2025 00:19
clintval
clintval reviewed Jan 22, 2025
View reviewed changes
prymer/offtarget/bwa.py Show resolved Hide resolved
prymer/offtarget/bwa.py Show resolved Hide resolved
prymer/offtarget/bwa.py Outdated Show resolved Hide resolved
prymer/offtarget/bwa.py Outdated Show resolved Hide resolved
@znorgaard
Copy link

Docstrings for prymer/offtarget/bwa.py and prymer/offtarget/offtarget_detector.py still have references to BwaAlnInteractive including a required install of bwa-aln-interactive.

@nh13
Copy link
Member Author

nh13 commented Jan 22, 2025 •
edited
Loading

@znorgaard

Docstrings for prymer/offtarget/bwa.py and prymer/offtarget/offtarget_detector.py still have references to BwaAlnInteractive including a required install of bwa-aln-interactive.

We have a list of deferred todos: #108

tfenne
tfenne approved these changes Jan 22, 2025
View reviewed changes
Copy link
Member

@tfenne tfenne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. There's more I'd like to do over time, but I think this is a great start!

README.md
The package `prymer` requires installation of [interactive `bwa`](https://github.com/fulcrumgenomics/bwa-aln-interactive).

To satisfy these requirements, it is recommended to install using [bioconda](https://bioconda.github.io/):
It is recommended to install using [bioconda](https://bioconda.github.io/):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once this PR is merged I hope to make a small PR to remove this statement, and fix up CONTRIBUTING to make it clear that you can develop prymer with just poetry available and managing the virtual envs.

prymer/util/executable_runner.py
@@ -1,160 +0,0 @@
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yesssssssssssss! So long ExecutableRunner!

@nh13 nh13 merged commit e2dc14a into dev Jan 22, 2025
2 of 3 checks passed
@nh13 nh13 deleted the feat/pybwa branch January 22, 2025 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@znorgaard znorgaard znorgaard left review comments

@clintval clintval clintval left review comments

@geoffjentry geoffjentry geoffjentry left review comments

@coderabbitai coderabbitai[bot] coderabbitai[bot] approved these changes

@tfenne tfenne tfenne approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nh13 @znorgaard @geoffjentry @tfenne @clintval