New classes DirectedHamiltonianCycle and CyclicTransitiveReduction #1008
Conversation
kriegaex
changed the title
|
Thanks for the contribution. For now, we can keep it open in PR form waiting for someone to add the unit tests and complete a review. In particular, it will be nice to get closer to moving all classes out of the .alg package (which we've completed for everything else), probably to a new .alg.transitive package. I expect @d-michail and @jkinable will have some comments about the literature/reference side (e.g. links to youtube videos tend to go stale). |
jgrapht-core/src/main/java/org/jgrapht/alg/CyclicTransitiveReduction.java
Outdated
Show resolved
Hide resolved
jgrapht-core/src/main/java/org/jgrapht/alg/TransitiveReduction.java
Outdated
Show resolved
Hide resolved
jgrapht-core/src/main/java/org/jgrapht/alg/CyclicTransitiveReduction.java
Show resolved
Hide resolved
jgrapht-core/src/main/java/org/jgrapht/alg/CyclicTransitiveReduction.java
Outdated
Show resolved
Hide resolved
|
@jsichi, I also would like to get your opinion about something I do not like much in the Hamiltonian cycle class hierarchy: You instantiate the algorithm, then you pass in the graph into |
|
What I also wanted to discuss: @d-michail mentioned in #667 (comment) that he did not like the singleton pattern in class |
This comment has been minimized.
This comment has been minimized.
|
@kriegaex regarding the HC invocation pattern, I understand your point. Most of our other algorithm interfaces follow the pattern you prefer (taking the graph in the constructor, not as a parameter to the execution method). Scanning the codebase, the reason HC is the way it is appears to be its usage in TwoOptHeuristicTSP (and maybe other places). TwoOptHeuristicTSP takes an instance of the HamiltonianCycleAlgorithm interface as one of its parameters. This allows the application to decide which HC algorithm implementation TwoOptHeuristicTSP should rely on. If the HC algorithm took the graph as its constructor parameter, then a factory would need to be supplied instead. If memory serves, we've discussed addressing this uniformly by introducing corresponding factory interfaces for all algorithms, but haven't yet got around to it, which is why the inconsistency you noticed remains. |
|
Regarding the singleton pattern: you've done it correctly (don't break the old code, but start following the desired pattern in new code). However, the new class should also live in a new package alg.transitive since we're trying to move out the remaining classes which currently live directly under .alg. For dealing with the old code, we follow a deprecation policy: https://github.com/jgrapht/jgrapht/wiki/Dev-guide%3A-Deprecation-policy So (as a separate PR) we would copy the old code to a new class in the .alg.transitive package, eliminating the singleton there. Then we would deprecate and eventually delete the old code entirely. |
jgrapht-core/src/main/java/org/jgrapht/alg/tour/DirectedHamiltonianCycle.java
Outdated
Show resolved
Hide resolved
|
Okay, here are the unit tests + some slower tests with bigger graphs, covering the two new algorithm classes. I hope they are sufficient and adequate, please review. Edit: One of the test cases for cyclic transitive reduction is identical to the sketch I posted in this comment. |
Yeah well, I thought I would better add links to my source even if it not a scientific paper but "just" a video. Basically, the algorithm for Hamiltonian cycle in a directed graph is a simple backtracking solution which probably is too trivial to have been described in any scientific paper. I found the source code for Martello's algorithm somewhere but I do not speak Fortran and would need a lot of time to try to understand and port it. Wolfram Mathworld mentions that the Wolfram Language (which I have zero experience with) offers several options called "Backtrack" (probably similar to mine), "Heuristic", "AngluinValiant", "Martello", and "MultiPath", but I do not know which one would perform better than exhaustive search backtracking and where to obtain sample source code or pseudo code algorithm descriptions which I could easily translate into a Java algorithm. I am not a scientist and cannot easily access academic sources. I am also not well versed in reading mathematical notation, but a pseudo code algorithm description is something I could work with. As I understand it, the worst case for all those methods would still be the same order of magnitude as backtracking even though in many (just not all) cases heuristics or other variants could shorten the runtime considerably. Feel free to correct me if I am talking complete nonsense. |
jgrapht-core/src/main/java/org/jgrapht/alg/CyclicTransitiveReduction.java
Outdated
Show resolved
Hide resolved
| * | ||
| * SPDX-License-Identifier: EPL-2.0 OR LGPL-2.1-or-later | ||
| */ | ||
| package org.jgrapht.alg; |
There was a problem hiding this comment.
Per my previous comment, this class should be placed in a new package org.jgrapht.alg.transitive. (Creating a new package involves adding a package-info.java in the new subdirectory, as well as adding a new export to jgrapht-core/src/main/java/module-info.java.)
There was a problem hiding this comment.
I leave this internal stuff to the maintainers. I have no desire to learn all your internal rules.
There was a problem hiding this comment.
Just curious: when you visit someone's house, if they ask you to hang up your coat in a particular closet, do you instead just throw it on the couch?
There was a problem hiding this comment.
No, but this is not the same situation. I am not visiting your house because you won't even let me enter the room. I would be much more inclined to hang my coat where you specify if I felt more welcome. Even then, as a host you could take my coat and hang it where and how you please.
Why are we even discussing this? This kind of refactoring can be done by a maintainer in any IDE in 10 seconds. I put my algorithms in the packages where similar ones already exist. If you want to refactor, do it after accepting the contribution. I know I am repeating myself, but you are micro-managing me, trying to pull my strings like I was a puppet. The debate takes much longer than you doing what you prefer after accepting the contribution.
I have reacted open-mindedly to many of your suggestions and implemented them. But this kind of stuff really is irrelevant to the functionality, it is purely a structural attribute. Maintainers should deal with that. Contributing to a project should be easy and not a big hurdle scaring off potential contributors because they surely won't try a second time. Don't make this feel like rocket science.
There was a problem hiding this comment.
Who are these maintainers of which you speak?
There was a problem hiding this comment.
Sorry for not explaining what I thought was an obvious term:
By maintainer I mean someone who has direct commit rights to this repository and/or by her code review is part of the decision if a contribution is to be accepted into the product or not. Someone with commit rights might also squash commits or otherwise modify PRs while/after accepting them. Maintainers may have other responsibilities, such as build and publish releases. By contrast, a contributor is someone like me, outside of the core team without any special rights or privileges, who contributes to the project by means of PRs, raising and commenting on issues, updating wiki pages etc.
Both maintainers and contributors can be rather inactive or regularly active, my differentiation is unrelated to the level of activity or number or frequency of contributions.
There was a problem hiding this comment.
We don't have any maintainers. We have committers. Committers review code and help contributors (via comments) to get it into suitable shape for being merged. That's our process, it's well documented, and it has worked quite effectively for us--take a look back over the history of all the other contributors whose work got merged. Since that process doesn't work for you, there's no more to be said.
But if you do find a maintainer with the necessary rights, please feel free to dictate to her whatever you think best.
There was a problem hiding this comment.
We don't have any maintainers. We have committers.
You call them committers, I call them maintainers because they do more than just commit. They effectively maintain the project.
if you do find a maintainer with the necessary rights, please feel free to dictate to her whatever you think best
Who is dictating? You put me in a position to either do exactly as you dictate or refuse my contribution due to minor structural issues we disagree upon and which you could easily fix by yourself after accepting the code. That is what a maintainer does where I come from. I am sorry to learn that this kind of thing is beneath you. If you want to lead, don't tell other people to run up a hill because you say so. Take the flag and walk first if you want to teach me. Don't whip me where you think is uphill.
"And we lead not by the example of our power, but by the power of our example."
Joe Biden
| * @throws IllegalArgumentException if graph allows multiple edges between two vertices | ||
| */ | ||
| public CyclicTransitiveReduction(Graph<V, E> directedGraph) { | ||
| if (directedGraph == null) |
There was a problem hiding this comment.
We use Objects.requireNonNull for this.
There was a problem hiding this comment.
But that throws a NPE and I want an IllegalArgumentException, as is clearly documented. So now what?
There was a problem hiding this comment.
Change your documentation to match the rest of our library?
There was a problem hiding this comment.
Firstly, like I said several times before, I do not know the rest of the library and have no desire to do so. I am a first-time user and also a first-time contributor, trying to do the project a favour.
Secondly, if I regard a null input graph an illegal argument, I will throw an IllegalArgumentException like I saw similar ones being thrown by other graph tests like e.g. GraphTests.requireDirected. I also think I am treating users kindly by documenting unchecked exceptions in this case.
This is not a documentation issue, it is about which exception to throw in a given situation.
There was a problem hiding this comment.
Yes, and accessing a null pointer should result in a NullPointerException, which is why Objects.requireNonNull is defined that way as part of the standard library.
jgrapht-core/src/main/java/org/jgrapht/alg/CyclicTransitiveReduction.java
Outdated
Show resolved
Hide resolved
jgrapht-core/src/main/java/org/jgrapht/alg/CyclicTransitiveReduction.java
Outdated
Show resolved
Hide resolved
jgrapht-core/src/test/java/org/jgrapht/alg/CyclicTransitiveReductionTest.java
Outdated
Show resolved
Hide resolved
jgrapht-core/src/test/java/org/jgrapht/alg/CyclicTransitiveReductionTest.java
Outdated
Show resolved
Hide resolved
| graph.addEdge("B", "C"); | ||
| graph.addEdge("C", "A"); | ||
| TransitiveReduction.INSTANCE.reduce(graph, false); | ||
| // CAVEAT: See how dangerous it is to call reduce(Graph, boolean) without the default cyclicity check? |
There was a problem hiding this comment.
The warning is worthwhile, but a test verifying incorrect behavior shouldn't be included. Maybe just move the warning to testAcyclicGraphNoCheck and invite the reader to investigate what happens if A->C is reversed.
There was a problem hiding this comment.
I disagree for two reasons:
- Tests for me do not just verify behaviour but are also a readable specification. When not forcing myself to use JUnit but in my own projects using the BDD tool Spock, test classes even inherit from a class called
Specification. - Verifying faulty behaviour in this case is helpful because if one day someone fixes the class under test or unifies the simple and directed transitive reduction classes into one (which I actually considered before deciding that I would leave that up to the maintainers), this test would fail and could be removed along with the comment.
There was a problem hiding this comment.
-
That's great, but it's meaningless to specify faulty behavior, unless you're working on some bug-for-bug-compatible emulator for a piece of legacy software.
-
Why wait when you can just remove it now?
There was a problem hiding this comment.
No, it is not meaningless because I am also documenting faulty behaviour that way. The test shows a use case in which the user can unintentionally mess up, using the parameter in the wrong situation. Arguably, someone could also write that into a wiki or user manual, but an executable test is more than just words, verifying facts which, if in the future they become obsolete, would make the test fail, triggering a developer possibly other than you or me to look at it and recognise that not only the test should be adapted or removed but also the documentation updated. Automate, automate, automate! You don't need to have active knowledge if the test shoves it in your face, maybe in 3 weeks, maybe in 3 years.
There was a problem hiding this comment.
This has stopped being fun quite a while ago. Anyway, thanks, I take your remark as an OK and will click "resolve" on this one.
I requested no such thing. You see a need for changes, @jsichi. I do not. In contrast to your statement, I requested multiple times that the PR be accepted as is and if someone feels a need to reformat, rename packages, change access modifiers and do other stuff of paramount importance, they can do so afterwards when the code is under their direct control and it is beyond my power to stop them. |
|
It's clear we've reached the end of productive dialogue, so I'm locking this PR for now. If someone else comes along and wants to continue the work to bring it to completion, please create a new issue linking here, and we'll unlock the conversation at that time. |
kriegaex
force-pushed
the
master
branch
2 times, most recently
from
66e6f72
to
79856fa
Compare
Also make sure that the simple TransitiveReduction class knows about its own limitation and throws an exception for cyclic digraphs.
If any given digraph is itself strongly connected or contains strongly connected components, the simple algorithm fails as documented. This was not tested before.
Warning was: invalid usage of tag <
…sses This is used in CyclicTransitiveReduction in order to avoid double checks in two distinct situations where it is already clear the the checked graph is acyclic.
…new ones This was a code review finding, thanks @jsichi!
Immutable data object containing basic information about an edge in a graph: the edge itself, the source vertex and the target vertex.
- <V, E> void addVertices(Graph<V, E> graph, V... vertices) - <V, E> void addHamiltonianCycle(Graph<V, E> graph, V... vertices) - <V, E> void randomiseGraph(Graph<V, E> graph)
As suggested in code review.
- long MathUtil.naturalNumberSumGauss(int n) - <V, E> void addEdges(Graph<V, E> graph, V... vertices)
Use 'addEdges(..)' to bulk-add edges instead of multiple 'graph.addEdge(..)' statements.
An SCC has either a single vertex and no edges or at least 3 vertices and 3 edges for a directed graph. Before the bugfix the algorithm was faulty insofar as it always assumed that an SCC has 3 edges and even asserted for it. When testing with another application, I noticed, then fixed the bug and added a test case for it.
Be more precise about synthetic edge mode. This was a code review finding.
This is more verbose when used in tests, but also makes them more readable. Furthermore, it does no longer require the check for an even number of varargs at the beginning of the method.
The way SCCs are now created makes it more difficult to find a Hamiltonian cycle. I did this in order to showcase the dramatic performance gain when using synthetic edges in cyclic transitive reduction. Typical runtimes on my workstation are: randomisedGraphsWithSCCsDifferentSizesNoSynthetic() -> 17,000 ms randomisedGraphsWithSCCsDifferentSizesSynthetic() -> 250 ms
- document new helper methods in TestUtils - rename 'addHamiltonianCycle' to 'addCycle' as suggested in the code review - similar to 'addCycle', 'addEdges' now automatically adds missing vertices, so in most cases tests do not have to call 'addVertices' first anymore
Code review finding
…tter The speed increase results from reducing SCC_MAX from 20 to 18, so the typical randomizedGraphsWithSCCsDifferentSizesNoSynthetic() now runs in about 2.5 s instead of 17-20 s before. Before removing intra-method timing log output for the net sum of reduce() calls as suggested during the code review, FWIW I also captured some typical measured runtimes in a data table inside a code comment.
Because of a rebase on the main branch after a 2-year hiatus, the tests previously implemented for JUnit 4 had to be migrated to JUnit Jupiter. This mostly affects assertion imports, expected exceptions and marking slow tests.
For a directed, unweighted graph with 3+ vertices, it is necessary to be strongly connected. But that does not mean, that every strongly connected such graph must have a HC. Fixes #1.
- Rename one local constant variable in a test - Exclude IntelliJ IDEA *.xml files from scanning
Currently this is a code-only contribution. I have tests locally, but not JUnit tests. I would have to transform them first and make them fit for the project. I am opening this PR with full knowledge that I am not meeting all of your PR requirements, but I or somebody else can refine the PR. I think the code is valuable enough as it is, there is enough Javadoc and it solves two open issues #667 and #1001. It would be nice to get feedback from maintainers as well as users. I just want to return something to this great project, so please don't bash me for making an imperfect or incomplete contribution. Nothing is more demotivating and I do hope that an imperfect contribution is better than none.
HISTORY.mdorCONTRIBUTORS.md