Implement Interval Graph data structures and detection algorithm

[pdelvo]

Hello!

Here is our implementation of a data structure for interval graphs (using a Red Black Tree internally) and the implementation of the detection algorithm presented in this paper. One problem with this is that the algorithm is not certifying for negative results and it cannot be easily extended to do so. The algorithm finds an ordering of the vertices that is, if the graph was an interval graph, an I-Ordering. The theory behind that states that a graph is an interval if and only if there exists an I-Ordering of the vertices. We want to also implement a certifying algorithm (e.g. the one that is based on PQ trees) but they are a bit more involved.

Fixes #545.

[jkinable]

Thanks for the submissions! Since this is a big PR, I'm afraid reviewing will take quite some time. However, before we can get started:

• Please resolve the existing merge conflicts
• Read https://github.com/jgrapht/jgrapht/wiki/How-to-make-your-first-%28code%29-contribution. There are still some things missing, such as the required headers in every class.
• Replace the EdgeFactories with Suppliers (see the release notes from our latest release: EdgeFactories became deprecated)
• I noticed that some classes have fairly little javadoc in the class description. It might be useful to enhance that a bit as well (including links to external websites).

[d-michail]

Great!

While resolving the conflicts, could you squash the commits (rebase and squash)?

[pdelvo]

I squashed everything and fixed the copyright headers

[jsichi]

What's the reason for copying all of the existing AbstractBaseGraph code? Most likely it's possible to accomplish the goal without reimplementing the Graph interface at all.

[d-michail]

Indeed, it is highly unlikely that you need to reimplement the graph interface or the graph specifics, etc.

[christophgruene]

Hi, i have implemented a big part of the data structures.

First of all, not all of the code is copied because adding of vertices is a special operation as we have to add edges regarding the given intervals. Furthermore, adding edges is not possible as they are defined by the given intervals. Of course this can easily be done by inheritance and overriding of the base implementation.
Nevertheless, the most important issue is the method removeVertex(V) in class AbstractBaseGraph. Here, a side effect is used to remove a vertex from the graph while the specifics are a private attribute in AbstractBaseGraph. Instead of calling specifics.getVertexSet().remove(V), one should call specifics.removeVertex(V), what we have done in IntervalGraph. This is important for our implementation of the interval graph since we have to remove the corresponding interval from the interval tree, too (see class org.jgrapht.graph.specifics.IntervalGraphVertexContainer). This issue can be solved by adaptation of these semantics to all graph classes that inherit AbstractBaseGraph.

[jsichi]

Use a graph listener for this purpose:

http://jgrapht.org/javadoc/org/jgrapht/event/GraphListener.html

[christophgruene]

We can use this interface, but I do not understand why. IntervalGraph is actually an instance of AbstractBaseGraph if the method removeVertex(V) is called in the specifics and not implicitly covered by specifics.getVertexSet().remove(V). Furthermore, this kind of implementation leads to more efficiency because such a listener is pure overhead as we still have to implement the specifics and the underlying data structures, which we need to maintain the connections of the vertices defined by the intervals. Thus, we just move the specifics to a different point of the architecture and all of those methods will be called. This will lead to an unstructured adaption of the specifics, which can be avoid by implementing new specifics.

The actual issue is the definition of subtypes of graphs. The library considers DirectedGraph, UndirectedGraph and MultiGraph as different subtypes, but there are no subtypes like interval graph, serial parallel graphs or other graphs that are build up by a certain structure. Therefore, it is very important for the future, how to handle such subtypes. Should they be implemented by using this Listener?

If you say yes, we will change the architecture; of course, this will take some time.

[jsichi]

Directed/undirected/multi are subtypes of fundamental graph structure, whereas interval graph, series/parallel graph, etc are mappings between graphs and other data structures. So no, I don't think we want to modify the core architecture to accommodate those.

Instead, JGraphT is designed for extensibility; so for a mapping such as interval graph, you should be able to proceed in the same way as if you were writing a JGraphT application without access to the internals. Listeners are one of the extensibility mechanisms we provide in order to allow you to keep your own data structure in sync with changes made to the graph. You can also use method overriding (as you've already done for preventing edge updates).

Regarding efficiency, we allow you to maintain whatever state you want in custom vertex/edge objects. And when that's not sufficient, usually Java HashMaps are suitable.

If, after implementing it that way, you use a Java profiler or other performance analysis tool and demonstrate an efficiency problem, then we can discuss how to optimize that.

[jsichi]

Regarding packaging, I suggest:

• Add a new package org.jgrapht.graph.interval for the new data structure
• Helper classes/interfaces which are specific to interval graphs (e.g. RedBlackIntervalTree) go in org.jgrapht.graph.interval
• Only general-purposes classes/interfaces (e.g. BinarySearchTree) go in org.jgrapht.util
• Rename org.jgrapht.alg.intervalgraph to org.jgrapht.alg.interval

@d-michail and @jkinable does this sound right?

[christophgruene]

Ok, thank you for your explanation. We will implement these things.

[christophgruene]

To avoid unnecessary work and communication, I want to ask, if I understand you correctly.
The class org.jgrapht.graph.interval (maybe, we change it to org.jgrapht.graph.IntervalGraphMapping as interval is used by us for a class representing the intervals) should contain a graph object as private attribute and we define all legal operations, e.g. addVertex(V), on the graph in this class intervalGraphMapping. Of course, every data structure, we need, will be an attribute of IntervalGraphMapping such that we can use them for these operations.
Furthermore, it should be possible to construct such a mapping from a given graph and for this case we should use a GraphListener to avoid the execution of illegal operations on the graph. What happens if someone modifies the edges in the underlying graph? Should we throw an exception after we have got a event from the graph listener? It is possible that the user only wants to modify the graph and does not want to consider that it is part of a mapping. Therefore, the mapping will be illegal and has to be destructed.

Thank you!

[jsichi]

Just to be clear on the naming: org.jgrapht.graph.interval would be the package name, so the class would be org.jgrapht.graph.interval.IntervalGraphMapping. Since you need a number of helper classes for the implementation, we want to keep them by themselves in a subpackage since the org.jgrapht.graph package is already very large.

Your understanding of my suggestion is correct. Note that the listener framework does not currently support enforcing constraints via throwing an exception (the event is fired after the graph is updated, and there is no transactional rollback implemented). It's theoretically possible to implement the rollback yourself in your listener, but it's likely to be problematic due to reentrancy and the possible presence of other listeners. So your idea about invalidating the mapping sounds like a good way to handle that case.

Also note that listeners are only supported when the original graph is a ListenableGraph; when that's not the case, the best you can do is produce a non-live mapping. Your documentation should make this clear.

As @jkinable mentioned, this is a big chunk of code, so you can expect code review to be a lengthy process. When you're in doubt about approaches, feel free to ask questions ahead of time (as you've already done) to avoid unnecessary effort. We appreciate your effort and patience!

Also, it's fine to defer some aspects to followup pull requests as long as each pull request submitted is coherent (i.e. in good shape for release) on its own. For example, if keeping the mapping live and valid via listeners is complicated, you can defer that to a followup pull request as long as your documentation makes the expected behavior of a non-live mapping clear.

[christophgruene]

We are almost finished with the implementation but now we have a problem.
The constructors of our class org.jgrapht.graph.interval.IntervalGraphMapping use DefaultListenableGraphs as discussed. For the construction of such a DefaultListenableGraph, we call the first constructor of the class

  /**
     * Creates a new listenable graph.
     *
     * @param g the backing graph.
     */
    public DefaultListenableGraph(Graph<V, E> g)
    {
        this(g, false);
    }

which calls

    /**
     * Creates a new listenable graph. If the <code>reuseEvents</code> flag is set to
     * <code>true</code> this class will reuse previously fired events and will not create a new
     * object for each event. This option increases performance but should be used with care,
     * especially in multithreaded environment.
     *
     * @param g the backing graph.
     * @param reuseEvents whether to reuse previously fired event objects instead of creating a new
     *        event object for each event.
     *
     * @throws IllegalArgumentException if the backing graph is already a listenable graph.
     */
    public DefaultListenableGraph(Graph<V, E> g, boolean reuseEvents)
    {
        super(g);
        this.reuseEvents = reuseEvents;
        reuseableEdgeEvent = new FlyweightEdgeEvent<>(this, -1, null);
        reuseableVertexEvent = new FlyweightVertexEvent<>(this, -1, null);

        // the following restriction could be probably relaxed in the future.
        if (g instanceof ListenableGraph<?, ?>) {
            throw new IllegalArgumentException("base graph cannot be listenable");
        }
    }

The problem is that GraphDelegator, which is the super class of DefaultListenableGraph, has two constructors, but we use the first one, such that the edgeSupplier is overwritten with null. This gives us a NullPointerException by using graph.addEdge(V, V).

/**
     * Constructor
     *
     * @param graph the backing graph (the delegate).
     */
    public GraphDelegator(Graph<V, E> graph)
    {
        this(graph, null, null);
    }

 
/**
     * 
     * @param graph the backing graph (the delegate).
     * @param vertexSupplier vertex supplier for the delegator. Can be null in which case the
     *        backing graph vertex supplier will be used.
     * @param edgeSupplier edge supplier for the delegator. Can be null in which case the backing
     *        graph edge supplier will be used.
     */
    public GraphDelegator(Graph<V, E> graph, Supplier<V> vertexSupplier, Supplier<E> edgeSupplier)
    {
        super();
        this.delegate = Objects.requireNonNull(graph, "graph must not be null");
        this.vertexSupplier = vertexSupplier;
        this.edgeSupplier = edgeSupplier;
    }

Is there something that we miss or should we correct this such that DefaultListenableGraph calls
public GraphDelegator(Graph<V, E>, Supplier<V>, Supplier<E>)
with suppliers from graph?

[jsichi]

GraphDelegator.addEdge delegates to the underlying graph (delegate) when its own edge supplier is null. So as long as the underlying graph implementation has an edge supplier, addEdge(V,V) should work fine.

[christophgruene]

Thank you for your help. Everything is fine now.

[christophgruene]

We added the IntervalGraphMapping as discussed and removed the IntervalGraph, IntervalSpecifics and IntervalGraphVertexContainer classes.

[jsichi]

@Toptachamann could you review the changes to LexBreadthFirstIterator? (If you have time to review more of the PR, that's great too.)

[jsichi]

@christophgruene @pdelvo since RedBlackTree is a general-purpose data structure, it should be in org.jgrapht.util. (Node should move along with it, renamed as RedBlackTreeNode.)

[jsichi]

There's no need to copy this giant Javadoc block.

[jsichi]

The Javadoc should make clear the semantics of the interval bounds (open/closed).

[jsichi]

Calling this class "IntervalVertex" is confusing because it makes it sound like instances are vertices in some graph, which is not the case. IntervalVertexPair would be good.

Also, I don't really see the need for a separate IntervalVertexInterface; there are no other implementations of this interface, so just use the class directly instead.

[jsichi]

As a general point, there's no need to include the word "Interface" in interface names. So e.g. "IntervalTreeInterface" should simply be named "IntervalTree".

Likewise for IntervalStructureInterface, although I'm not clear on the need for this (and class IntervalTreeStructure) to exist at all; it seems to just add a bit of wrapping on top of IntervalTree.

[jsichi]

Why does a reference to the comparator have to be stored along with each key? Isn't the same comparator used for the entire tree? If that's true, then this class should not exist at all, and you should just use T as the key directly.

[jsichi]

Isn't this redundant with size() on any of the ArrayLists above?

[jsichi]

I guess I'm wondering why you can't just use LinkedHashSet for this. All of the necessary operations are constant-time (you can use an iterator for peek/poll), it already has the necessary doubly-linked list logic, and its memory usage will be a lot better for large numbers of vertices.

[danielmock]

Thank you for your input. We were not aware of the LinkedHashSet! I adapted the code now to work with the LinkedHashSet (see last commits).
However, we are unsatisfied that we have to precompute the ordered adjacency lists for each vertex. (It is necessary that the splitterVertices for the split method are sorted by the same ordering as the LinkedHashList, otherwise the resulting bucket would not have the same order).

Is there another way to do this?

[jsichi]

I'm having trouble understanding your question with reference to the latest version of the code (where the split method is gone). Could you point me to the lines you are referring to for

• precomputation
• splitting

?

[danielmock]

Sure. For the LBFS variants LBFS+ and LBFS*, the order inside the buckets has to be preserved. Hence when updateBuckets(v) is invoked (for a vertex v), the neighbors of v have to be processed by this order to guarantee that the newly added buckets are again sorted by this order.

For this the neighborhoods have to be sorted. Since we want to have linear time complexity, it seems necessary to do this at the beginning of the algorithm (see computeSortedNeighborhoods). This results in having a higher space consumption.

This does not affect the implementation of the "normal" LBFS algorithm.

[jkinable]

I've left several remarks. In addition I noticed that there are verious remarks made by @jsichi which haven't been resolved. Please go ahead and fix those. We are working towards a new release and it would be nice to add interval graph support.

After my basic remarks have been fixed, I'll go over it in more detail to identify performance issues.

[jkinable]

final

[jkinable]

why not use the primitive type?

[jkinable]

why initialize with a size of 0?

[jkinable]

let's use the standard naming using by other algorithms: lazy...

Also, I would not store the result. For this particular class it's quite weird. just perform the computations when the user invokes isIntervalGraph(). There's no sensible reason for the user to invoke isIntervalGraph() twice on the same graph.

[danielmock]

Other algorithms store their results as well.
Additionally, it enables the user to get other attributes without having to compute the same thing over and over again.

We could put the computation into the constructor, but I do not think that is a good idea.

Which name do you propose?

[jkinable]

This is not what you are checking in the if statement: you are checking for strict inequality, not equality. Also, this particular implementation does not support empty intervals, i.e. an interval where start==end.
Could you also implement a convenience method isEmpty() which returns true if the interval is empty?

[danielmock]

We wanted that intervals satisfy start <= end. Assuming this, I don't see a problem with the if condition (the negation is strict).
Additionally, an interval with start == end is not empty (assuming that the intervals are closed).

[christophgruene]

I did not changed the behavior of the intervals in the last commits. What is the status of this request? Shall we implement an empty interval or not? An empty interval in an interval graph will result in an isolated vertex, which can be achieved by adding non-intersecting intervals as well.

[jkinable]

Objects.requireNonNull

[jkinable]

This implementation can be optimized, resulting in fewer comparison.
First check for left. If left then return. Then check right. If right then return. Otherwise return 0 (they must overlap)

[jkinable]

Must implement serializable

[jkinable]

Use math notation: https://github.com/jgrapht/jgrapht/wiki/How-to-write-documentation#math-in-javadoc

[jkinable]

I'm a bit puzzled as to what exactly this is. Is this some data structure for your algorithm? If so, make it package private. Is this something a user is supposed to use? If so, add proper documentation. If you use this as a simple pair, use the Pair class in jgrapht.

[christophgruene]

This class is used in the interval graph data structures to have a container that contains an interval (in order to construct the interval graph) and a vertex type as we have in a normal graph.

[christophgruene]

One question arises: org.jgrapht.Graph provides a method setEdgeWeight and getEdgeWeight, which manage the weights of edges independent on the edge type E. Thus, we have overall three possibilities to implement interval support for vertices, whereby the first one maps the behavior of edges in Graph:

1. We introduce a "IntervalSpecifics" class for IntervalGraph that allows to get the interval representing some vertex of type V by implementing getInterval() in IntervalGraph.
2. We use this very IntervalVertexPair class, to provide a container and we have IntervalGraph<V, T, E> and the vertices are IntervalVertexPairs<V, T> (that is the status quo in our implementations)
3. We add an interface "IntervalVertex" such that IntervalGraph<V extends IntervalVertex, E>, that is, a vertex in an interval graph implements this interface. Thus certain methods such as getInterval() or the like are guaranteed to exist in a vertex.

[christophgruene]

Better late than never, I reacted to the latest change requests. I am sorry for that! To bring this PR to life again, I suggest that we first bring this PR into the library and after that I will open the PR for the interval graph data structures as suggest by @jsichi earlier. Your comments are welcome.

[Toptachamann]

In order to start the review process, please:

• rebase your branch on top the most recent changes of the master branch.
• make sure you address all existing PR comments so that the same things aren't discussed several times.

[Toptachamann]

@christophgruene could you please let me know if there're any updates on this PR?

[christophgruene]

@christophgruene could you please let me know if there're any updates on this PR?

@Toptachamann I reacted to all comments in the code but one; for IntervalVertexPair.java I added a comment in the last change request. Furthermore, I rebased the branch. That is, there is nothing left to do from my side for now.

[jkinable]

I recently needed an interval tree for a project, and I ran into this github project: https://github.com/lodborg/interval-tree
They have a very nice implementation of an Interval: https://github.com/lodborg/interval-tree/blob/master/src/main/java/com/lodborg/intervaltree/Interval.java
I would recommend using that interval representation.

[Toptachamann]

Thanks! All right, I've marked the unresolved comments (their main author is @jsichi). Please, address all the comments. If some comments are unclear to you, please mark them. Resolving these comments is an essential task before proceeding with a further PR review (benchmarks, code correctness and remaining comments).

[Toptachamann]

This comment doesn't seem to be addressed

[Toptachamann]

We don't use tag anymore

[christophgruene]

What do you mean with tag? Probably html-tag?

[Toptachamann]

Substitute an array with a list. This way you remove the need to suppress a warning. Moreover, later in the code, you convert this array to a list anyway. As a general note, avoid using arrays of generic elements in Java.

[Toptachamann]

We recently added CollectionUtil, use it instead.

[Toptachamann]

This comment isn't addressed

[Toptachamann]

[Toptachamann]

Remove all commented code

[Toptachamann]

Remove

[Toptachamann]

Remove

[Toptachamann]

No connect to the parameter and no authorship note.

[christophgruene]

@jkinable Is it possible to copy the code for the interval or do I need to rewrite the code? I am not sure whether the license allows that.

[jkinable]

@jkinable Is it possible to copy the code for the interval or do I need to rewrite the code? I am not sure whether the license allows that.

@jsichi any idea? Their code is released under the MIT license which is very permissive. I presume we can use it without alteration, as long as we retain their copyright. We could drop a note in their repo.

[jsichi]

Their library is published to Maven Central, so if we're going to use it, we should add a dependency rather than copying any code:

https://search.maven.org/artifact/com.lodborg/interval-tree/1.0.0/jar

Depending on an MIT-licensed library is fine.

This might be overkill for just interval, but if we're eventually planning to revisit the original scope of this PR (which included an interval graph data structure), then it would probably make sense.

[christophgruene]

I reacted to all comments. Some comments have responses, which were not addressed until now. Thus they are not reflected in the code.

I am not sure, how to proceed concerning the interval implementation. I left it like this at the moment and come back if the data structures are discussed.