-
Notifications
You must be signed in to change notification settings - Fork 38
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
Overeager link-adding in odgi flip
#496
Comments
Interesting! Can I ask some follow-up questions to ensure that I'm understanding what's interesting about these examples?
|
Thanks for this, Adrian! tl;dr: I think leaving it as-is would be fine, I just worry about downstream reliances on this "fixing" behavior.
|
anshumanmohan
added a commit
to cucapra/pollen
that referenced
this issue
Apr 18, 2023
We agree with odgi on all the handmade files except note5.gfa I think odgi is doing something fishy there; see pangenome/odgi#485 (comment) We agree with odgi on all the handmade files except flip4.gfa flip4 is not a valid graph and so arguably it does not matter what we do with it, but `odgi flip` does currently _fix_ flip4, so I think it's worth keeping around. The point of divergence is that we do not currently fix graphs like flip4, though it's easy enough to do if that were desired. See pangenome/odgi#496
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I think
flip
is adding new links too eagerly.Minimal
Here's a minimal example:
Clearly the path does not require a flip. More subtly, though, the graph is not well-formed: it will fail
validate
. Runningflip
and thenview
has an interesting effect:Now this graph is valid! Interesting, but IMO not
flip
's job!Why this exists
I'll create a valid graph that is in need of a flip:
And now
flip
and thenview
shows a reasonable output: a path was flipped, and two links were added in support of the new path.Fix
The paths that have just been generated need to be siloed off, and only links that are needed by those paths should be added.
Something else is also going on
I do still think that
flip
is doing something else that's a bit fishy. See note5.gfa from your test suite. I don't think that fixing the above would fixflip
's behavior when run against note5. See my comment here: #485 (comment)The text was updated successfully, but these errors were encountered: