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.

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

drop in xg0 (take two) #2402

Closed
wants to merge 85 commits into from
Closed

drop in xg0 (take two) #2402

wants to merge 85 commits into from

Conversation

glennhickey
Copy link
Contributor

@glennhickey glennhickey commented Aug 16, 2019

This is a fresh stab at #2311, made much easier by all the handlefication efforts in the meantime.

As of this commit, it's compiling but not linking due to a couple trailing XGPath usages..

@glennhickey
Copy link
Contributor Author

Having the libhandlegraph version specified in two places

is a wee bit dangerous (a little mismatch gave me weird segfaults)

@glennhickey
Copy link
Contributor Author

From the first failed bash tap test, it looks like creating an xg index is dropping a bunch of paths

vg construct -m 1000 -r small/xy.fa -v small/xy2.vcf.gz -R x -C -a 2> /dev/null | vg view -j - | sed s/_alt/alt/g | vg view -Jv - >w.vg
vg paths -v w.vg -L | wc -l
39
vg index w.vg -x w.xg
vg paths -x w.xg -L | wc -l
30

@ekg
Copy link
Member

ekg commented Aug 16, 2019 via email

@glennhickey
Copy link
Contributor Author

Yeah, this error seems to be coming from empty paths getting dropped by XG::from_path_handle_graph(). Pretty minor, I think (provided the format supports empty paths).

@glennhickey
Copy link
Contributor Author

Most of these changes are now in master via #2409. All the test failures in this PR are therefore due to either

  • bugs in new xg
  • subtle build problems (My main suspect, as everything runs fine when I build locally)
  • bugs in VGSet::to_xg() (only vg code change here that's not in master)

A future PR adding the new xg should only need to adjust the Makefile and submodules as well as the aforementioned VGSet function. Hopefully the third time is the charm. Anyone else wanna give it a shot? @adamnovak @ekg ?

@adamnovak
Copy link
Member

I'm going to set up a PR that makes your other changes on top of this. We will see how that goes.

@glennhickey glennhickey mentioned this pull request Sep 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants