Add basic functionality to read indexed FASTA files (continued) by landesfeind · Pull Request #214 · rust-bio/rust-htslib

landesfeind · 2020-06-14T13:35:58Z

This pull request supersedes the #165 in using an own branch for easier integration. Most changes of @dlaehnemann (see #165 review) are incorporated.

#165 (review)

dlaehnemann · 2020-06-14T17:56:07Z

To make the clippy errors go away and make this pull request work, I think you have to add #include "htslib/htslib/faidx.h" to the include list at the start of hts-sys/wrapper.h:

rust-htslib/hts-sys/wrapper.h

Lines 1 to 9 in 61b0644

    
           #include "htslib/htslib/hts.h" 
        
           #include "htslib/htslib/vcf.h" 
        
           #include "htslib/htslib/sam.h" 
        
           #include "htslib/htslib/cram.h" 
        
           #include "htslib/htslib/bgzf.h" 
        
           #include "htslib/htslib/vcfutils.h" 
        
           #include "htslib/htslib/tbx.h" 
        
           #include "htslib/htslib/synced_bcf_reader.h" 
        
           #include "htslib/htslib/kbitset.h"

… faidx

landesfeind · 2020-06-14T18:55:41Z

Hm, that one is actually there: https://github.com/landesfeind/rust-htslib/blob/faidx/hts-sys/wrapper.h#L10
Also, I ran the full CI pipeline in my own repository before I submitted the pull-request: https://github.com/landesfeind/rust-htslib/actions/runs/135013019

Let's try with a merging of the master branch.

dlaehnemann · 2020-06-14T19:33:18Z

You're right. Overlooked that, as well... But it still can't find the relevant structs and functions, weird. I guess merging in the master is always worth a try...

landesfeind · 2020-06-14T21:15:54Z

Do the *_prebuilt_bindings.rs probably miss faidx?
After merging the master, also my local build fails using cargo build. But it works well when enforcing generation of the bindings using cargo build --features bindgen.
Any chance you can give me a hint how the prebuilt are update? I compared the bindings.rs generated during my "bindgen-build" with the prebuilt one but there are many many differences. I think I can not simply copy them over.

dlaehnemann · 2020-06-15T09:56:17Z

From looking at it, this does seem like the problem. Check out the new info at the end of the README.md section on Usage of rust-htslib. It's because using bindgen is now an optional feature, which requires prebuilt bindings for all functionality that is to work without bindgen. For context, you could checkout PR #189 which was merged 3 days ago.
But it looks like for the faidx functions and structs, this can easily be modelled upon existing code, right? Otherwise just ping in pmarks and brainstorm, they should know much more about this than I do.

pmarks · 2020-06-15T20:47:53Z

@landesfeind I will add updated bindings to include faidx stuff & send you a branch that you can merge into this. (I'll also include instructions for how to do the update).

pmarks · 2020-06-16T22:00:44Z

@landesfeind try merging my branch here into this branch -- that should fix build without bindgen.

landesfeind · 2020-06-18T18:52:37Z

Hi @pmarks, that did the trick! Thanks a lot! Ready for merge, unless there are more ideas on code improvement :-)

@brainstorm

As requested by @brainstorm (see #214 (comment))

Internal C method faidx_fetch_seq64 uses i64 as type to index into large FASTA sequences. The fetch function now returns a byte array instead of a String to be more efficient. A new method was added to automatically convert to a String for convenience.

The return type of the faidx functions is changed to a Result capturing a potential failure in converting the usize into a i64 (hts_pos_t in the C API).

brainstorm

Thanks Manuel! Just the minor typo and we should be good to go IMHO.

@johanneskoester , seems that manual migrated to thiserror now, any other comments on this one?

brainstorm · 2020-08-04T01:32:53Z

Thanks for your contribution @landesfeind 👏🏻

landesfeind · 2020-08-04T18:44:30Z

You are welcome - thank you all for the feedback!

landesfeind added 6 commits June 10, 2020 21:15

Cherry pick the pull request for faidx

47355a3

#165 (review)

Cargo formatting

4780c0c

Update faidx documentation for clarification

64da403

Implement faidx_open test

e9d2148

Run actions on faidx branch

4531a4d

Remove faidx from github actions

29565f9

landesfeind mentioned this pull request Jun 14, 2020

Add basic functionality to read indexed FASTA files #165

Closed

dlaehnemann mentioned this pull request Jun 14, 2020

create src/errors.rs with generic errors for the entire crate #215

Closed

landesfeind added 2 commits June 14, 2020 20:53

Merge branch 'master' of https://github.com/rust-bio/rust-htslib into…

67d03d6

… faidx

Merge branch 'faidx' of github.com:landesfeind/rust-htslib into faidx

a29369a

Activate CI for faidx branch

79bbf8f

landesfeind mentioned this pull request Jun 15, 2020

Features & build cleanups #189

Merged

2 tasks

pmarks and others added 2 commits June 16, 2020 14:56

update osx bindings with faidx support

8f7f789

update linux bindings for faidx

a58680d

brainstorm requested changes Jun 22, 2020

View reviewed changes

Comment thread .github/workflows/rust.yml Outdated

Drop faidx branch from CI actions

bea3e9f

As requested by @brainstorm (see #214 (comment))

johanneskoester requested changes Jun 22, 2020

View reviewed changes

Comment thread src/faidx/errors.rs Outdated

johanneskoester requested changes Jun 22, 2020

View reviewed changes

Comment thread src/faidx/mod.rs Outdated

landesfeind added 4 commits June 23, 2020 20:21

Merge remote-tracking branch 'upstream/master' into faidx

16692a3

Migrate faidx to use thiserror instead of snafu (#214 (comment))

78d0e84

Formatting using cargo fmt

b4c265b

Merge branch 'faidx' of github.com:landesfeind/rust-htslib into faidx

cbab854

landesfeind requested review from brainstorm and johanneskoester June 27, 2020 12:11

brainstorm and others added 2 commits July 2, 2020 13:36

Merge branch 'master' into faidx

f735af8

Use usize-type for position paramter in faidx

c6dc913

The return type of the faidx functions is changed to a Result capturing a potential failure in converting the usize into a i64 (hts_pos_t in the C API).

brainstorm requested changes Jul 20, 2020

View reviewed changes

Comment thread src/faidx/errors.rs Outdated

brainstorm and others added 3 commits July 20, 2020 15:50

Merge branch 'master' into faidx

242db0c

Correct typo in faidx-error: to -> too

ac5aa20

Merge branch 'faidx' of github.com:landesfeind/rust-htslib into faidx

33dba58

brainstorm approved these changes Aug 4, 2020

View reviewed changes

brainstorm merged commit 4a44444 into rust-bio:master Aug 4, 2020

landesfeind deleted the faidx branch August 4, 2020 18:44

Conversation

landesfeind commented Jun 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlaehnemann commented Jun 14, 2020

Uh oh!

landesfeind commented Jun 14, 2020

Uh oh!

dlaehnemann commented Jun 14, 2020

Uh oh!

landesfeind commented Jun 14, 2020

Uh oh!

dlaehnemann commented Jun 15, 2020

Uh oh!

pmarks commented Jun 15, 2020

Uh oh!

pmarks commented Jun 16, 2020

Uh oh!

landesfeind commented Jun 18, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brainstorm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brainstorm commented Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

landesfeind commented Aug 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

landesfeind commented Jun 14, 2020 •

edited

Loading

brainstorm commented Aug 4, 2020 •

edited

Loading