Autoindex oversized

[faithokamoto]

Changelog Entry

To be copied to the draft changelog by merger:

• Help vg autoindex not error when indexing a graph with oversized snarls

Description

Okay, so this doesn't actually work yet, but it's a step towards addressing #4724. Last August, vgteam/libbdsg#221 changed bdsg so that is_regular_snarl() could handle distanceless distance indexes. However, that managed to break stuff for indexes that had distances but also had oversized snarls. In particular, causing this error:

    if ((record_type == SNARL || record_type == OVERSIZED_SNARL) && graph == nullptr) {
        throw runtime_error("error: is_regular_snarl requires a graph if the distance index doesn't contain distances");
    }

Great, I thought, why not just pass a graph? If this was trickling down from vg autoindex, we already had such a graph in a calling function. I had that graph handed down the line. And now we no longer get the same error. Instead, we get a different error! The very next one:

    if ((record_type == SNARL || record_type == OVERSIZED_SNARL) && !allow_internal_loops) {
        throw runtime_error("error: is_regular_snarl requires distances in the distance index to verify that there are no internal loops");
    }

So, uh, yeah. This is still broken. I wanted to document the test case and my attempt at fixing it, though. Possible ways to fix the next error include:

• passing allow_internal_loop = true; before we were had the default of false, so when I had to be explicit about it I just used that, but we could change it
• dropping OVERSIZED_SNARL from the types that trigger the new error; they do store distances to the boundary nodes so that should let us calculate whether there's a self-loop, right?

@xchang1 thoughts?

[xchang1]

I haven't really had a chance to look at this closely yet but to your last two points:

• I think I made allow_internal_loops=false the default because that's what it was before I added it. I was worried that making it true by default would break something. But maybe the right thing to do is not give it a default value and make everything that doesn't pass a value false.
• Yeah, that looks right to me.

[faithokamoto]

Messed a bit with libbdsg and now my local version passes the test case I had set up. Will wait for the other PR to pass CI & be merged, then I'll tag in the new libbdsg version here.

[faithokamoto]

Actually, bumping the libbdsg here first, then I can merge together. I think my PR over there looks fine but I'd rather see it pass CI here first.

[faithokamoto]

I merged the libbdsg PR, but Adam bumped that dependency separately. I did merge magic and this shouldn't have conflicts any more.