Made hbond graph pyrosetta compatible

[bcov77]

You can't actually use the HBondGraph in pyrosetta because the LowMemGraph uses some crazy tricks that don't involve object inheritance.

This adds 2 functions to bridge that gap.

[roccomoretti]

This is unsafe if the node isn't actually a HBondNode. Please use dynamic_cast instead. If you pass a LowMemNode which isn't an HBondNode, dynamic_cast will raise a (C++) exception, rather than giving you invalid memory access.

[bcov77]

So, I tried that first and it doesn't compile. I think if that did work then pyrosetta wouldn't have an issue with this.

I'm happy to format this however necessary though to make it safer. I just think that the way this is set up is actually very inherently unsafe.

/mnt/home/bcov/rosetta_green/rosetta/source/src/core/scoring/hbonds/graph/HBondGraph.cc:29:39: error: cannot dynamic_cast 'node' (of type 'class utility::graph::LowMemNode') to type 'class core::scoring::hbonds::graph::HBondNode&' (source type is not polymorphic)

[roccomoretti]

Ah, crud. The issue is that (to save space) LowMemNode doesn't have any virtual functions. This means there isn't a vtable and thus dynamic_cast doesn't have any information about what the class "actually" is to check it. -- I think you're correct that this is also the source of your issue with PyRosetta not being able to convert them properly. There's no way at runtime to see if the LowMemNode is actually a HBondNode, so the binder code refuses to make the (potentially unsafe) conversion.

I'm not really liking having this function be something at the general/global scope. I'm concerned it's something that might be used indiscriminately, which leads to memory corruption and crashes if you're not overly careful and accidentally use it in the wrong context.

I think the reason we can get away with not having virtual functions and dynamic typing here is that the Nodes and Edges are something of an "internal" detail of the Graph structure. The Graph knows what the real type of its internal nodes are, so it's safe to do unconditional conversions, so long as it's only done on the objects the HBondGraph knows are its own.

Where is the Node object you're using coming from? If we can move that original access to the HBondGraph object proper, then we can have it hand out the HBondNode directly (as the cast would be safe). Heck, even if you need to keep it as a conversion function, I'd probably recommend making it a member function for the HBondGraph, where you do a safety check first to make sure that the passed LowMemNode is contained within the graph member vector (e.g. with pointer comparisons).

Also, it's best to use static_cast for these sorts of unconditional casts, as that stands out more to people doing safety checks, versus the not-as-grep-able C-style cast.

[bcov77]

Ha, you've finally discovered the LowMemGraph class :P

I mean, I find it hard to believe anyone would use this function by accident. Pyrosetta still type checks that you're passing a LowMemNode/LowMemEdge. To use this by accident you'd have to somehow create a LowMemGraph and then think you can type-cast your stuff to another class by discovering this function. It feels like only an expert level pyrosetta coder could make such a mistake.

Maybe I could add a warning docstring and rename this to unsafe_HBondNode_from_LowMemNode (in addition to static_cast<>)? I could also make it a static method of LowMemEdge and LowMemNode if you think that's more hidden.

Where are these coming from? You're right that in theory this is possible, but I'd basically have to fully-reimplement the base class in all derived classes. If you compare LowMemGraph.hh and HBondGraph.hh, you'll see that all of the getters (and iterators) are in LowMemGraph.hh and return LowMemGraphs. I don't think there's an elegant way to do this (as anyone that calls this in C++ is expected to static_cast the results, you just can't do it in pyrosetta).

[roccomoretti]

Hmm, I guess you're right. My inclination is still to make this a instance method on the HBondGraph class. Presumably if you're dealing with HBondNodes, you'll have the associated HBondGraph class kicking around. We should be able to support the functionality while still being safe. (Agreed that it's an odd edge case where someone is using a non-HBondGraph LowMemGraph and would feed it to this function, but given that if they did it would result in memory corruption, I think we should avoid it if possible.)

I think something like the following should work (written but not tested):

HBondNode & 
HBondGraph::HBondNode_from_LowMemNode(utility::graph::LowMemNode & node) {
    for (core::Size ii(1); ii <= num_nodes(); ++ii ) {
        HBondNode * hbnode = get_node(ii); // Templated base class gives correct derived type
        if ( (&node) == static_cast<utility::graph::LowMemNode *>( hbnode )  ) { // Compare object identity via pointers
            return *hbnode;
        }
    }
    // Couldn't find it.
    utility_exit_with_message("Attempted to convert a LowMemNode to an HBondNode through an HBondGraph which doesn't contain the node.");
}

[bcov77]

Wow! Amazing! Thank you. I would have never come up with that.