The ODGI graph does not pass the handle graph tests ported over from VG

[jeizenga]

I haven't looked into this deeply yet, but we should figure out whether this is the fault of the tests or the implementation, or perhaps a disagreement over the generality of our formalism.

Currently, I've commented out ODGI in these tests so that they would still run for the other graphs.

[ekg]

Odgi test calls a version of these tests from a few months ago. What failed?

…

On Sat, Aug 24, 2019, 02:17 Jordan Eizenga ***@***.***> wrote: I haven't looked into this deeply yet, but we should figure out whether this is the fault of the tests or the implementation, or perhaps a disagreement over the generality of our formalism. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#19?email_source=notifications&email_token=AABDQEJIC5QSYNL554AUEQDQGB4YZA5CNFSM4IPESWL2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HHEZAYQ>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABDQEJKFS63645MNGLP4RLQGB4YZANCNFSM4IPESWLQ> .

[jeizenga]

Okay, I think I at least have a clue what's happening. It relates to this line in the orginal node.cpp:
https://github.com/vgteam/odgi/blob/master/src/node.cpp#L36

I think I wrote to you about this on slack a while back. The issue is that the assert statement was never actually executing because of this line in the CMakeLists, which removes all asserts
https://github.com/vgteam/odgi/blob/master/CMakeLists.txt#L235

I had to edit the line to have edge_count() instead of edge_count in order to compile. However, this assert statement has probably never actually run in the tests in the odgi repository because of that cmake flag. According to the slack messages I left you, you should be able to reproduce this from the vg view GFA from tiny.vg.

[ekg]

I get it now. I forgot that release removed asserts. This assertion is incorrect and should be removed or changed. The length of res should be edge_count*EDGE_RECORD_LENGTH.

…

On Sun, Aug 25, 2019, 23:59 Jordan Eizenga ***@***.***> wrote: Okay, I think I at least have a clue what's happening. It relates to this line in the orginal node.cpp: https://github.com/vgteam/odgi/blob/master/src/node.cpp#L36 I think I wrote to you about this on slack a while back. The issue is that the assert statement was never actually executing because of this line in the CMakeLists, which removes all asserts https://github.com/vgteam/odgi/blob/master/CMakeLists.txt#L235 I had to edit the line to have edge_count() instead of edge_count in order to compile. However, this assert statement has probably never actually run in the tests in the odgi repository because of that cmake flag. According to the slack messages I left you, you should be able to reproduce this from the vg view GFA from tiny.vg. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#19?email_source=notifications&email_token=AABDQELL3MCJ4AQGRJJQHETQGL6CVA5CNFSM4IPESWL2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5C4W2Q#issuecomment-524667754>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABDQEJAY2HUBVEZNE42IC3QGL6CVANCNFSM4IPESWLQ> .

[jeizenga]

Another update: if I comment out that assert everything passes. Is that assert statement correct?

[ekg]

No, as I described, the assert is not correct. It was left over from a previous version. It just needs to be removed.

…

On Mon, Aug 26, 2019, 07:36 Jordan Eizenga ***@***.***> wrote: Another update: if I comment out that assert everything passes. Is that assert statement correct? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#19?email_source=notifications&email_token=AABDQEMYN6KWR7ROCTJAB6TQGNTVZA5CNFSM4IPESWL2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5DKBVA#issuecomment-524722388>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABDQEMW33QBTWITLMZCGI3QGNTVZANCNFSM4IPESWLQ> .

[jeizenga]

Oh yeah, sorry I posted without refreshing the page. I've started activating more tests and found some differences in how we've implemented circular paths that are causing ODGI to fail some more of the tests I wrote.

The tests are written with the assumption that circular paths "loop around" when calling get_next/previous_step or has_next/previous_step. I've tried modifying your implementation to have this behavior, but it seems that has_next/previous_step are used pretty substantially in internal routines. Other things seem to break, but I'm not totally sure why yet.

[jeizenga]

@ekg I think I might need your help resolving this. I tried to add the "looping around" behavior for circular paths like this:

bool ODGI::has_next_step(const step_handle_t& step_handle) const {
    const node_t& node = node_v.at(number_bool_packing::unpack_number(get_handle_of_step(step_handle)));
    return (node.get_path_step(as_integers(step_handle)[1]).next_id() != path_end_marker ||
            get_is_circular(as_path_handle(node.get_path_step(as_integers(step_handle)[1]).path_id())));
}

It looks right to me, but it seems to completely blow up the destroy_step method, which uses has_next and has_previous. However, strangely the problem seems to occur on a non-circular path, where I would expect to function the same as previously. Perhaps the graph entered an invalid state before this?

[ekg]

Ok I'll take a look. How do I get to the same state as you?

…

On Tue, Aug 27, 2019, 16:06 Jordan Eizenga ***@***.***> wrote: @ekg <https://github.com/ekg> I think I might need your help resolving this. I tried to add the "looping around" behavior for circular paths like this: bool ODGI::has_next_step(const step_handle_t& step_handle) const { const node_t& node = node_v.at(number_bool_packing::unpack_number(get_handle_of_step(step_handle))); return (node.get_path_step(as_integers(step_handle)[1]).next_id() != path_end_marker || get_is_circular(as_path_handle(node.get_path_step(as_integers(step_handle)[1]).path_id()))); } It looks right to me, but it seems to completely blow up the destroy_step method, which uses has_next and has_previous. However, strangely the problem seems to occur on a non-circular path, which I would expect to function the same as previously. Perhaps the graph entered an invalid state before this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19?email_source=notifications&email_token=AABDQELT6H62YS6FZIC4S5TQGWCKZA5CNFSM4IPESWL2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5I6O4A#issuecomment-525461360>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABDQEIYOQX6U3DBIJES3C3QGWCKZANCNFSM4IPESWLQ> .

[jeizenga]

Probably the easiest way is with the easy install repo: https://github.com/vgteam/libbdsg-easy
Clone it, checkout b942cc96c8 in the libbdsg submodule, and then use the make in the root directory. It should automatically execute the tests.