jguhlin / minimap2-rs

Rust bindings to minimap2 library
Other
51 stars 11 forks source link

FEATURE: implement with_seq for single sequences #27

Closed jrharting closed 1 year ago

jrharting commented 1 year ago

This would be hugely helpful -- I use it all the time in the py bindings, would prefer rust!

jguhlin commented 1 year ago

Hey. I'm out of town but will get to this once I'm back in March. Should be easy enough. Cheers

On Sat, Feb 18, 2023, 5:51 AM John Harting @.***> wrote:

This would be hugely helpful -- I use it all the time in the py bindings, would prefer rust!

— Reply to this email directly, view it on GitHub https://github.com/jguhlin/minimap2-rs/issues/27, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOA2CHIISGK5BA4AOE6XYLWX7CDLANCNFSM6AAAAAAU7YI45I . You are receiving this because you are subscribed to this thread.Message ID: @.***>

jrharting commented 1 year ago

thank you so much!

jguhlin commented 1 year ago

Hey @jrharting, I've got this working in the github branch main. Any chance you could test it before I roll it out? Otherwise happy to roll it out in the next few days. I've got a simple test there, but would be good to get some feedback first if possible.

jrharting commented 1 year ago

Hi @jguhlin, thanks for this! I can definitely give this a spin this week -- I'm working on something that this is perfect for right now.

jguhlin commented 1 year ago

Great! The latest git should have it. There is an option to add more than 1 sequence with this API call in minimap2, but it's hardcoded to 1 in python (and I don't think available at all in the CLI). Let me know if that'd be useful and I can implement it quick today.

jrharting commented 1 year ago

Having an option to add >1 sequence would be potentially useful -- I'd be happy to check that out if its easy to implement

jguhlin commented 1 year ago

Ok, it did not work. Signal 11. with_seq and with_seq_and_id are both implemented (just changes the N/A id to whatever you specify). Let me know how it works out, and any ergonomic problems. Happy to take pull requests or try to change the types to be more consistent.

I may try to reimplement with_seqs for multiple ones later, it looks like it should work but doesn't. Hmm

jrharting commented 1 year ago

The aligner is looking good so far! I've only been using the "with_seq" mode (no ID), and it looks good at first look. I did notice a couple "println!" statements in the github version that kick out the seq and ID (eg https://github.com/jguhlin/minimap2-rs/blob/main/src/lib.rs#L623), but no worries for testing purposes.

One quick question that came up -- how can i change just the k-mer size for the index opts, but otherwise use the short-read settings? I tried something like this, but I think the sr() call at the end overwrites the initial declaration of k.

        let aligner = Aligner {
            mapopt: MapOpt {
                best_n: 1,
                ..Default::default()
            },
            idxopt: IdxOpt {
                k: 7,
                ..Default::default()
            },
            threads: 8,
            ..Default::default()
        }
        .sr()
        .with_cigar()
        .with_seq(seq)
        .expect("Unable to build aligner index");
jguhlin commented 1 year ago

Bah, I always leave my debugging code everywhere. For changing the settings, you'd have to do the sr() first, then make the changes later. There isn't plumbing for the builder pattern there, but I could add those functions in. Let me investigate some.

jguhlin commented 1 year ago

For the configuration, I think setting the preset then adjusting the params would be best. So

        let mut sr = Aligner::builder().sr();
        sr.mapopt.best_n = 1; // Default already I believe
        sr.idxopt.k = 7;

But if you did want to hold to that pattern, you could do:

        let aligner = Aligner {
            mapopt: MapOpt {
                best_n: 1,
                ..Aligner::builder().sr().mapopt
            },
            idxopt: IdxOpt {
                k: 7,
                ..Aligner::builder().sr().idxopt
            },
            ..sr
        };
jrharting commented 1 year ago

Thanks! The aligner config worked great.

So far I have not seen any issues with the "with_seq" aligners using short and long reads. Is there anything in particular I could test that would be useful for you?

jguhlin commented 1 year ago

Hey, that's great to hear. I've got some tests up, and will plan to release a new version this week or next (a Mac compilation bug just showed up, so that'd be good to fix before a release as well).

Good luck, if you build something public with it let me know and I'll add it to the README

wdecoster commented 1 year ago

with_seq yields a 'not yet implemented' panic in v0.1.11 - do you have a timeframe for when that feature would be available through crates.io?

jguhlin commented 1 year ago

Will be soon. Rolling out a new version soon to go with the new minimap release. Hopefully a few days

On Thu, 4 May 2023, 9:49 pm Wouter De Coster, @.***> wrote:

with_seq yields a 'not yet implemented' panic in v0.1.11 - do you have a timeframe for when that feature would be available through crates.io?

— Reply to this email directly, view it on GitHub https://github.com/jguhlin/minimap2-rs/issues/27#issuecomment-1534437092, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOA2CCQF7QS4I3WXDRMIETXEN3SNANCNFSM6AAAAAAU7YI45I . You are receiving this because you modified the open/close state.Message ID: @.***>

jrharting commented 7 months ago

Hi again!

Thanks for all the work you've done for minimap2-rs! I wanted to let you know that I just made the first public release of hifihla -- a tool for calling HLA star-alleles from PacBio HiFi data. minimap2-rs is a core library in this tool -- it would not be the same without it! Thank you so much for all the work you did to make this available!

Here is a link to the tool -- please add it to the list of users of your excellent library. https://github.com/PacificBiosciences/hifihla

Thanks! John

jguhlin commented 7 months ago

@jrharting Hey, that's great! I've added it. Glad it worked out! :) My first attempt at an FFI binding and working deeper with alignment tools.

Cheers, --Joseph