Re-opened previous PR with suggested improvements and fixes. by tomnelson · Pull Request #185 · jrtom/jung

tomnelson · 2018-01-13T13:26:31Z

This is mostly about LayoutModel::getLocations() but also has additional javadocs

Changes to be committed: modified: jung-algorithms/src/main/java/edu/uci/ics/jung/algorithms/util/IterativeContext.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/algorithms/CircleLayoutAlgorithm.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/algorithms/FRBHVisitorLayoutAlgorithm.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/algorithms/FRLayoutAlgorithm.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/algorithms/ISOMLayoutAlgorithm.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/algorithms/IterativeLayoutAlgorithm.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/algorithms/KKLayoutAlgorithm.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/algorithms/LayoutAlgorithm.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/algorithms/RadialTreeLayoutAlgorithm.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/algorithms/SpringBHVisitorLayoutAlgorithm.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/algorithms/SpringLayoutAlgorithm.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/algorithms/StaticLayoutAlgorithm.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/algorithms/TreeLayoutAlgorithm.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/model/DefaultLayoutModelChangeSupport.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/model/LayoutModel.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/model/LoadingCacheLayoutModel.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/model/Point.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/model/PolarPoint.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/spatial/BarnesHutQuadTree.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/spatial/Circle.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/spatial/Node.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/spatial/Rectangle.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/util/NetworkNodeAccessor.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/util/VisRunnable.java modified: jung-algorithms/src/test/java/edu/uci/ics/jung/layout/algorithms/FRBHIteratorLayoutAlgorithm.java new file: jung-algorithms/src/test/java/edu/uci/ics/jung/layout/algorithms/ForceObjectIterator.java modified: jung-algorithms/src/test/java/edu/uci/ics/jung/layout/algorithms/SpringBHIteratorLayoutAlgorithm.java modified: jung-algorithms/src/test/java/edu/uci/ics/jung/layout/spatial/FRLayoutsTest.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/BarnesHutVisualizer.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/NodeCollapseDemo.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/BasicVisualizationServer.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/layout/AggregateLayoutModel.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/layout/AnimationLayoutAlgorithm.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/spatial/AbstractSpatial.java modified: tools/deploy_snapshots.sh modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/util/NetworkNodeAccessor.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/util/RadiusNetworkNodeAccessor.java

Changes to be committed: modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/model/AbstractLayoutModel.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/model/DefaultLayoutModelChangeSupport.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/model/DefaultLayoutStateChangeSupport.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/model/LayoutModel.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/model/LoadingCacheLayoutModel.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/model/Point.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/model/PolarPoint.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/spatial/Node.java modified: jung-algorithms/src/test/java/edu/uci/ics/jung/layout/spatial/FRLayoutsTest.java modified: jung-visualization/src/main/java/edu/uci/ics/jung/visualization/layout/AggregateLayoutModel.java

Changes to be committed: modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/algorithms/FRBHVisitorLayoutAlgorithm.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/model/AbstractLayoutModel.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/model/LayoutModel.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/model/LoadingCacheLayoutModel.java

Changes to be committed: modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/algorithms/CircleLayoutAlgorithm.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/algorithms/FRBHVisitorLayoutAlgorithm.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/algorithms/SpringBHVisitorLayoutAlgorithm.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/model/AbstractLayoutModel.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/model/LayoutModel.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/model/LoadingCacheLayoutModel.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/spatial/BarnesHutQuadTree.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/spatial/Node.java modified: jung-algorithms/src/main/java/edu/uci/ics/jung/layout/util/RadiusNetworkNodeAccessor.java modified: jung-samples/src/main/java/edu/uci/ics/jung/samples/BarnesHutVisualizer.java

…ances. Sometimes the test would fail with very close floating point numbers Changes to be committed: renamed: jung-algorithms/src/test/java/edu/uci/ics/jung/layout/spatial/CircleTest.java -> jung-algorithms/src/test/java/edu/uci/ics/jung/layout/spatial/ShapesTest.java modified: jung-visualization/src/test/java/edu/uci/ics/jung/visualization/spatial/rtree/RTreeTest.java

tomnelson · 2018-01-13T15:29:03Z

jung-visualization/src/test/java/edu/uci/ics/jung/visualization/spatial/rtree/RTreeTest.java

+      }
+      return -1;
+    }
+  }


This hack is because the test would sometimes fail (as in your most recent push) because of comparing floating point values like this (in italics):
Failed tests: testOne(edu.uci.ics.jung.visualization.spatial.rtree.RTreeTest): expected:<java.awt.geom.Rectangle2D$Double[x=2.9168632067831934,y=0.1405494536367291,w=570.535341820459,h=599.8594505463633]> but was:<java.awt.geom.Rectangle2D$Double[x=2.9168632067831934,y=0.1405494536367291,w=570.5353418204589,h=599.8594505463633]>

Yep. I think I actually expressed some concern about the direct comparison of doubles in some previous PR; I don't remember the exact context, but in general we probably don't want to be doing that.

Changes to be committed: modified: jung-visualization/src/test/java/edu/uci/ics/jung/visualization/spatial/rtree/RTreeTest.java

jrtom

Looks pretty good, just a few cleanup comments. Thanks for sticking with this. :)

Also, I am really happy with how the getLocations() thing worked out, that's just a much nicer API all around. :)

jrtom · 2018-01-13T23:26:32Z

...-algorithms/src/main/java/edu/uci/ics/jung/layout/model/DefaultLayoutModelChangeSupport.java

 import org.slf4j.LoggerFactory;

+/**
+ * an Event system for LayoutModels to announce the change in the location of Nodes This replaces


punctuation missing

see other comment about event system

jrtom · 2018-01-13T23:30:21Z

...-algorithms/src/main/java/edu/uci/ics/jung/layout/algorithms/FRBHVisitorLayoutAlgorithm.java

 /**
 * This subclass of FRLayoutAlgorithm applies a Barnes-Hut QuadTree optimization during the
- * calculation of node repulsion
+ * calculation of node repulsion. The purpose of the Barnes-Hut optimization is to reduce the n-body


I would say rather than "the n-body problem" "the number of calculations required"

and here i wuz getting all graph-theory-like.....

IME "n-body problem" is from physics, but OK :)

jrtom · 2018-01-13T23:37:02Z

jung-algorithms/src/main/java/edu/uci/ics/jung/layout/model/LayoutModel.java

+  /**
+   * Changes the layout coordinates of {@code node} to {@code x, y}.
+   *
+   * @param node


missing descriptions for params

jrtom · 2018-01-13T23:38:31Z

jung-algorithms/src/main/java/edu/uci/ics/jung/layout/model/LayoutModel.java

-   * This exists so that LayoutModel will not have dependencies on java awt or swing event classes
-   */
-  interface ChangeSupport {
+  ChangeSupport getChangeSupport();


missing Javadoc

i have an event redesign ready to go. This will be OBE :)

I have no idea what you mean by OBE, but I assume that this means that this will be changing. :)

overcome by events

jrtom · 2018-01-13T23:42:53Z

jung-algorithms/src/main/java/edu/uci/ics/jung/layout/model/Point.java

+  /**
+   * @param ox coordinate of another location
+   * @param oy coordinate of another location
+   * @returnthe square of the distance between this Point and the passed location


missing space after @return

jrtom · 2018-01-13T23:56:26Z

jung-samples/src/main/java/edu/uci/ics/jung/samples/BarnesHutVisualizer.java

-  MutableNetwork<String, Number> network;
  BarnesHutQuadTree<String> tree;

+  Map<String, Point> elements = Maps.newHashMap();


Style nit: this (and the line below) are fine, but now that the diamond operator is available (as of Java 7) it's actually just as convenient to do

Map<String, Point> elements = new HashMap<>();

and uses slightly fewer characters if you care about that sort of thing.

The main reason why the parameterless Maps.newHashMap() and Sets.newHashSet() were created was to get around the requirement for redundantly specifying the generic types.

interesting. I would have bet that you'd go for the factory method over the CTOR any day.

In this context I don't have a strong preference. I'm pretty sure that those specific factory methods would never have existed if it weren't for the former redundant type spec requirement, though.

jrtom · 2018-01-14T00:00:14Z

jung-visualization/src/test/java/edu/uci/ics/jung/visualization/spatial/rtree/RTreeTest.java

+  }
+
+  private boolean closeEnough(double left, double right) {
+    return Math.abs(left - right) < 0.001;


Please define a constant for the tolerance.

I'm delighted with your idea for getLocations too.

jrtom · 2018-01-14T00:04:44Z

jung-visualization/src/test/java/edu/uci/ics/jung/visualization/spatial/rtree/RTreeTest.java

+
+  private boolean closeEnough(Rectangle2D left, Rectangle2D right) {
+    return left.equals(right)
+        || (closeEnough(left.getMinX(), right.getMinX())


I would be inclined to use the Truth library for this; jbduncan@ and I will probably convert most/all of the JUNG test assertions to use Truth once things settle down a bit. Specifically, Truth provides facilities for comparing double values within a specified tolerance: https://google.github.io/truth/floating_point

sounds like something for a future PR. I like that idea.

I like this idea too. I'll go and open an issue on this for now, if we don't already have one already, just so we don't lose track of it. :)

@jbduncan I pushed a commit to use 'truth' in my latest PR. Please have a look to see if it can be improved. I noticed that there are a number of other places in jung-algorithms legacy code where a tolerance is used in a unit test comparison. Using 'truth' really cleans up the code. Kudos to the author(s) for making it so readable.

@tomnelson I've had a look and made some suggestions! Please let me know what you think over there. :)

tomnelson added 6 commits January 12, 2018 12:58

revert competing change

6238353

tomnelson commented Jan 13, 2018

View reviewed changes

replaced the comparator with a more direct approach

086ff4a

Changes to be committed: modified: jung-visualization/src/test/java/edu/uci/ics/jung/visualization/spatial/rtree/RTreeTest.java

jrtom reviewed Jan 14, 2018

View reviewed changes

cleanup

85756d7

jrtom merged commit f11345d into jrtom:master Jan 14, 2018

jrtom mentioned this pull request Jan 14, 2018

provide a Map<N, Point> getLocations() method on LayoutModel #184

Closed

jbduncan mentioned this pull request Jan 14, 2018

Migrate from JUnit assertions to Truth #188

Open

Conversation

tomnelson commented Jan 13, 2018

Uh oh!

tomnelson Jan 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrtom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tomnelson Jan 13, 2018 •

edited

Loading