Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactoring ChordalityInspector and interface revision #547

Merged
merged 11 commits into from
May 3, 2018
Merged

Refactoring ChordalityInspector and interface revision #547

merged 11 commits into from
May 3, 2018

Conversation

jkinable
Copy link
Collaborator

@jkinable jkinable commented Apr 12, 2018 •
edited

  • Moved CycleDetector to org.jgrapht.alg.cycle package
  • Refactored ChordalityInspector (moved coloring, clique and independent set methods to their own classes/packages).
  • Added ChordalGraphColoring, ChordalGraphMaxCliqueFinder, ChordalGraphIndependentSetFinder and corresponding tests.
  • Added interfaces CliqueAlgorithm, IndependentSetAlgorithm, VertexCoverAlgorithm
  • Added WeightedUnmodifiableSet as a generic container for weighted sets (e.g. max weight clique)
  • Removed TSPAlgorithm as it has been replaced by HamiltonianCycleAlgorithm
  • Replaced MinimumWeightedVertexCoverAlgorithm and MinimumVertexCoverAlgorithm interfaces by one unified VertexCoverAlgorithm interface which is similar in structure as CliqueAlgorithm and IndependentSetAlgorithm
  • SpannerAlgorithm.Spanner now behaves the same as VertexCoverAlgorithm.VertexCover

To simplify reviewing, no modifications to the algorithms have been made.

@jkinable jkinable requested a review from d-michail April 12, 2018 17:38
@d-michail
Copy link
Member

Regarding the interfaces I personally prefer to not add method getGraph(). Usually the user knowns the graph that she/he is working on. The role of these objects is mostly of a DTO (Data Transfer Object) and not of a "context".

Regarding implementation, if we want to replace them with something which behaves like a Set<V>, I would suggest to implement a class WeightedUnmodifiableSet in the util package (using class AbstractSet as base). Then only extend that class and add a constructor. This way we can avoid the code duplication.

@jkinable jkinable changed the title Refactoring ChordalityInspector and added new algorithmic interfaces Refactoring ChordalityInspector and interface revision Apr 14, 2018
@jkinable
Copy link
Collaborator Author

jkinable commented Apr 14, 2018 •
edited

Regarding implementation, if we want to replace them with something which behaves like a Set, I would suggest to implement a class WeightedUnmodifiableSet in the util package (using class AbstractSet as base). Then only extend that class and add a constructor. This way we can avoid the code duplication.

@d-michail good suggestion. I implemented the WeightedUnmodifiableSet and revised various algorithmic interfaces accordingly. The behavior of these interfaces is now much more homogeneous. Let me know what you think.

Toptachamann
Toptachamann reviewed Apr 17, 2018
View reviewed changes
jgrapht-core/src/main/java/org/jgrapht/alg/clique/ChordalGraphMaxCliqueFinder.java Outdated
}

/**
* Lazily computes some maximum clique of the {@code graph}. Returns null if the graph isn't chordal.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method doesn't return anything.

Toptachamann
Toptachamann reviewed Apr 17, 2018
View reviewed changes
jgrapht-core/src/main/java/org/jgrapht/alg/independentset/ChordalGraphIndependentSetFinder.java Outdated

/**
* Lazily computes a maximum independent set of the inspected {@code graph}.
* If the graph isn't chordal, returns null.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same here

@Toptachamann
Copy link
Collaborator

Some method documentation needs to be updated, cause the methods don't return anything anymore.

@jkinable jkinable mentioned this pull request Apr 18, 2018
Closed
@jkinable
Copy link
Collaborator Author

Any more comments on this one?

d-michail
d-michail reviewed Apr 28, 2018
View reviewed changes
jgrapht-core/src/main/java/org/jgrapht/alg/shortestpath/TreeSingleSourcePathsImpl.java Outdated
@@ -135,7 +135,7 @@ public double getWeight(V targetVertex)
}

double weight = 0d;
while (p != null && !p.equals(source)) {
while (p != null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkinable The proposed fix in #489 is correct. We still need this check since there might be an entry also for the source vertex containing the tuple (0, null). We should merge PR #489 instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I stand corrected. Thanks for the clarification. Updated my branch accordingly.

@jkinable
Copy link
Collaborator Author

jkinable commented May 3, 2018

any additional requests?

@d-michail
Copy link
Member

Looks good!

@d-michail d-michail merged commit eeee1be into jgrapht:master May 3, 2018
@mrizzi mrizzi mentioned this pull request Jan 23, 2019
Merged
@syoon2 syoon2 mentioned this pull request Apr 12, 2024
Open
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d-michail d-michail d-michail left review comments

@Toptachamann Toptachamann Toptachamann left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jkinable @d-michail @Toptachamann