Merge pull request #3745 from soerenreichardt/prevent-negative-histogram-values

jjaderberg · web-flow · commit 3de7face7f60 · 2021-09-09T13:48:37.000+02:00
Prevent computing histogram when using log scaler
diff --git a/algo-common/src/main/java/org/neo4j/gds/result/AbstractCentralityResultBuilder.java b/algo-common/src/main/java/org/neo4j/gds/result/AbstractCentralityResultBuilder.java
@@ -20,40 +20,32 @@
 package org.neo4j.gds.result;
 
 import org.HdrHistogram.DoubleHistogram;
-import org.jetbrains.annotations.Nullable;
+import org.jetbrains.annotations.NotNull;
 import org.neo4j.gds.compat.MapUtil;
 import org.neo4j.gds.core.concurrency.Pools;
 import org.neo4j.gds.core.utils.ProgressTimer;
 import org.neo4j.gds.core.utils.statistics.CentralityStatistics;
+import org.neo4j.gds.scaling.ScalarScaler;
 import org.neo4j.internal.kernel.api.procs.ProcedureCallContext;
 
+import java.util.HashMap;
 import java.util.Map;
 import java.util.Optional;
 import java.util.function.LongToDoubleFunction;
 
 public abstract class AbstractCentralityResultBuilder<WRITE_RESULT> extends AbstractResultBuilder<WRITE_RESULT> {
 
-    private final int concurrency;
-    protected boolean buildHistogram;
+    private static final String HISTOGRAM_ERROR_KEY = "Error";
 
-    protected long postProcessingMillis = -1L;
-    protected Optional<DoubleHistogram> maybeCentralityHistogram = Optional.empty();
+    private final int concurrency;
+    private final boolean buildHistogram;
+    private final Map<String, Object> histogramError;
 
-    protected @Nullable Map<String, Object> centralityHistogramOrNull() {
-        return maybeCentralityHistogram.map(histogram -> MapUtil.map(
-            "min", histogram.getMinValue(),
-            "mean", histogram.getMean(),
-            "max", histogram.getMaxValue(),
-            "p50", histogram.getValueAtPercentile(50),
-            "p75", histogram.getValueAtPercentile(75),
-            "p90", histogram.getValueAtPercentile(90),
-            "p95", histogram.getValueAtPercentile(95),
-            "p99", histogram.getValueAtPercentile(99),
-            "p999", histogram.getValueAtPercentile(99.9)
-        )).orElse(null);
-    }
+    private LongToDoubleFunction centralityFunction;
+    private ScalarScaler.Variant scaler;
 
-    protected LongToDoubleFunction centralityFunction = null;
+    protected long postProcessingMillis = -1L;
+    protected Map<String, Object> centralityHistogram;
 
     protected AbstractCentralityResultBuilder(
         ProcedureCallContext callContext,
@@ -63,6 +55,7 @@ protected AbstractCentralityResultBuilder(
             .outputFields()
             .anyMatch(s -> s.equalsIgnoreCase("centralityDistribution"));
         this.concurrency = concurrency;
+        this.histogramError = new HashMap<>();
     }
 
     protected abstract WRITE_RESULT buildResult();
@@ -72,17 +65,57 @@ public AbstractCentralityResultBuilder<WRITE_RESULT> withCentralityFunction(Long
         return this;
     }
 
+    public AbstractCentralityResultBuilder<WRITE_RESULT> withScalerVariant(ScalarScaler.Variant scaler) {
+        this.scaler = scaler;
+        return this;
+    }
+
     @Override
     public WRITE_RESULT build() {
-        final ProgressTimer timer = ProgressTimer.start();
+        var timer = ProgressTimer.start();
+        var maybeCentralityHistogram = computeCentralityHistogram();
+        this.centralityHistogram = centralityHistogramResult(maybeCentralityHistogram);
 
-        if (buildHistogram && centralityFunction != null) {
-            maybeCentralityHistogram = Optional.of(CentralityStatistics.histogram(nodeCount, centralityFunction,Pools.DEFAULT, concurrency));
-        }
         timer.stop();
         this.postProcessingMillis = timer.getDuration();
 
         return buildResult();
     }
 
+    @NotNull
+    private Optional<DoubleHistogram> computeCentralityHistogram() {
+        var logScaler = scaler == ScalarScaler.Variant.LOG;
+        if (buildHistogram && centralityFunction != null) {
+            if (logScaler) {
+                logScalerHistogramError();
+            } else {
+                return Optional.of(CentralityStatistics.histogram(
+                    nodeCount,
+                    centralityFunction,
+                    Pools.DEFAULT,
+                    concurrency
+                ));
+            }
+        }
+        return Optional.empty();
+    }
+
+    private Map<String, Object> centralityHistogramResult(Optional<DoubleHistogram> maybeHistogram) {
+        return maybeHistogram.map(histogram -> MapUtil.map(
+            "min", histogram.getMinValue(),
+            "mean", histogram.getMean(),
+            "max", histogram.getMaxValue(),
+            "p50", histogram.getValueAtPercentile(50),
+            "p75", histogram.getValueAtPercentile(75),
+            "p90", histogram.getValueAtPercentile(90),
+            "p95", histogram.getValueAtPercentile(95),
+            "p99", histogram.getValueAtPercentile(99),
+            "p999", histogram.getValueAtPercentile(99.9)
+        )).orElse(histogramError);
+    }
+
+    private void logScalerHistogramError() {
+        this.histogramError.put(HISTOGRAM_ERROR_KEY, "Unable to create histogram when using scaler of type " + ScalarScaler.Variant.LOG);
+    }
+
 }
diff --git a/alpha/alpha-algo/src/main/java/org/neo4j/gds/results/CentralityScore.java b/alpha/alpha-algo/src/main/java/org/neo4j/gds/results/CentralityScore.java
@@ -20,8 +20,8 @@
 package org.neo4j.gds.results;
 
 import org.jetbrains.annotations.Nullable;
-import org.neo4j.gds.result.AbstractCentralityResultBuilder;
 import org.neo4j.gds.config.WritePropertyConfig;
+import org.neo4j.gds.result.AbstractCentralityResultBuilder;
 import org.neo4j.internal.kernel.api.procs.ProcedureCallContext;
 
 import java.util.Map;
@@ -73,7 +73,7 @@ public CentralityScore.Stats buildResult() {
                     computeMillis,
                     writeMillis,
                     config instanceof WritePropertyConfig ? ((WritePropertyConfig) config).writeProperty() : "",
-                    centralityHistogramOrNull()
+                    centralityHistogram
                 );
             }
         }
diff --git a/alpha/alpha-algo/src/main/java/org/neo4j/gds/results/PageRankScore.java b/alpha/alpha-algo/src/main/java/org/neo4j/gds/results/PageRankScore.java
@@ -20,8 +20,8 @@
 package org.neo4j.gds.results;
 
 import org.jetbrains.annotations.Nullable;
-import org.neo4j.gds.result.AbstractCentralityResultBuilder;
 import org.neo4j.gds.config.WritePropertyConfig;
+import org.neo4j.gds.result.AbstractCentralityResultBuilder;
 import org.neo4j.internal.kernel.api.procs.ProcedureCallContext;
 
 import java.util.Map;
@@ -92,7 +92,7 @@ public Stats buildResult() {
                     writeMillis,
                     dampingFactor,
                     config instanceof WritePropertyConfig ? ((WritePropertyConfig) config).writeProperty() : "",
-                    centralityHistogramOrNull()
+                    centralityHistogram
                 );
             }
         }
diff --git a/proc/centrality/src/main/java/org/neo4j/gds/betweenness/BetweennessCentralityMutateProc.java b/proc/centrality/src/main/java/org/neo4j/gds/betweenness/BetweennessCentralityMutateProc.java
@@ -137,7 +137,7 @@ public MutateResult buildResult() {
                     computeMillis,
                     postProcessingMillis,
                     mutateMillis,
-                    centralityHistogramOrNull(),
+                    centralityHistogram,
                     sumCentrality,
                     minCentrality,
                     maxCentrality,
diff --git a/proc/centrality/src/main/java/org/neo4j/gds/betweenness/BetweennessCentralityStatsProc.java b/proc/centrality/src/main/java/org/neo4j/gds/betweenness/BetweennessCentralityStatsProc.java
@@ -128,7 +128,7 @@ protected Builder(ProcedureCallContext callContext, int concurrency) {
             @Override
             public StatsResult buildResult() {
                 return new StatsResult(
-                    centralityHistogramOrNull(),
+                    centralityHistogram,
                     sumCentrality,
                     minCentrality,
                     maxCentrality,
diff --git a/proc/centrality/src/main/java/org/neo4j/gds/betweenness/BetweennessCentralityWriteProc.java b/proc/centrality/src/main/java/org/neo4j/gds/betweenness/BetweennessCentralityWriteProc.java
@@ -132,7 +132,7 @@ public WriteResult buildResult() {
                     computeMillis,
                     postProcessingMillis,
                     writeMillis,
-                    centralityHistogramOrNull(),
+                    centralityHistogram,
                     sumCentrality,
                     minCentrality,
                     maxCentrality,
diff --git a/proc/centrality/src/main/java/org/neo4j/gds/degree/DegreeCentralityMutateProc.java b/proc/centrality/src/main/java/org/neo4j/gds/degree/DegreeCentralityMutateProc.java
@@ -127,7 +127,7 @@ public DegreeCentralityMutateProc.MutateResult buildResult() {
                     computeMillis,
                     postProcessingMillis,
                     mutateMillis,
-                    centralityHistogramOrNull(),
+                    centralityHistogram,
                     config.toMap()
                 );
             }
diff --git a/proc/centrality/src/main/java/org/neo4j/gds/degree/DegreeCentralityStatsProc.java b/proc/centrality/src/main/java/org/neo4j/gds/degree/DegreeCentralityStatsProc.java
@@ -112,7 +112,7 @@ protected Builder(ProcedureCallContext callContext, int concurrency) {
             @Override
             public DegreeCentralityStatsProc.StatsResult buildResult() {
                 return new DegreeCentralityStatsProc.StatsResult(
-                    centralityHistogramOrNull(),
+                    centralityHistogram,
                     createMillis,
                     computeMillis,
                     postProcessingMillis,
diff --git a/proc/centrality/src/main/java/org/neo4j/gds/degree/DegreeCentralityWriteProc.java b/proc/centrality/src/main/java/org/neo4j/gds/degree/DegreeCentralityWriteProc.java
@@ -123,7 +123,7 @@ protected WriteResult buildResult() {
                     computeMillis,
                     postProcessingMillis,
                     writeMillis,
-                    centralityHistogramOrNull(),
+                    centralityHistogram,
                     config.toMap()
                 );
             }
diff --git a/proc/centrality/src/main/java/org/neo4j/gds/pagerank/PageRankMutateProc.java b/proc/centrality/src/main/java/org/neo4j/gds/pagerank/PageRankMutateProc.java
@@ -138,7 +138,7 @@ public MutateResult buildResult() {
                 return new MutateResult(
                     ranIterations,
                     didConverge,
-                    centralityHistogramOrNull(),
+                    centralityHistogram,
                     createMillis,
                     computeMillis,
                     postProcessingMillis,
diff --git a/proc/centrality/src/main/java/org/neo4j/gds/pagerank/PageRankProc.java b/proc/centrality/src/main/java/org/neo4j/gds/pagerank/PageRankProc.java
@@ -19,9 +19,9 @@
  */
 package org.neo4j.gds.pagerank;
 
-import org.neo4j.gds.result.AbstractCentralityResultBuilder;
 import org.neo4j.gds.AlgoBaseProc;
 import org.neo4j.gds.api.NodeProperties;
+import org.neo4j.gds.result.AbstractCentralityResultBuilder;
 import org.neo4j.internal.kernel.api.procs.ProcedureCallContext;
 import org.neo4j.logging.Log;
 
@@ -47,7 +47,8 @@ static <PROC_RESULT, CONFIG extends PageRankConfig> PageRankResultBuilder<PROC_R
         procResultBuilder
             .withDidConverge(!computeResult.isGraphEmpty() && result.didConverge())
             .withRanIterations(!computeResult.isGraphEmpty() ? result.iterations() : 0)
-            .withCentralityFunction(!computeResult.isGraphEmpty() ? computeResult.result().scores()::get : null);
+            .withCentralityFunction(!computeResult.isGraphEmpty() ? computeResult.result().scores()::get : null)
+            .withScalerVariant(computeResult.config().scaler());
 
         return procResultBuilder;
     }
diff --git a/proc/centrality/src/main/java/org/neo4j/gds/pagerank/PageRankStatsProc.java b/proc/centrality/src/main/java/org/neo4j/gds/pagerank/PageRankStatsProc.java
@@ -124,7 +124,7 @@ public StatsResult buildResult() {
                 return new StatsResult(
                     ranIterations,
                     didConverge,
-                    centralityHistogramOrNull(),
+                    centralityHistogram,
                     createMillis,
                     computeMillis,
                     postProcessingMillis,
diff --git a/proc/centrality/src/main/java/org/neo4j/gds/pagerank/PageRankWriteProc.java b/proc/centrality/src/main/java/org/neo4j/gds/pagerank/PageRankWriteProc.java
@@ -139,7 +139,7 @@ public WriteResult buildResult() {
                 return new WriteResult(
                     ranIterations,
                     didConverge,
-                    centralityHistogramOrNull(),
+                    centralityHistogram,
                     createMillis,
                     computeMillis,
                     postProcessingMillis,
diff --git a/proc/centrality/src/test/java/org/neo4j/gds/pagerank/PageRankWriteProcTest.java b/proc/centrality/src/test/java/org/neo4j/gds/pagerank/PageRankWriteProcTest.java
@@ -23,11 +23,12 @@
 import org.junit.jupiter.api.TestFactory;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.MethodSource;
+import org.neo4j.gds.AlgoBaseProc;
+import org.neo4j.gds.GdsCypher.ModeBuildStage;
 import org.neo4j.gds.WritePropertyConfigProcTest;
 import org.neo4j.gds.compat.MapUtil;
 import org.neo4j.gds.core.CypherMapWrapper;
-import org.neo4j.gds.AlgoBaseProc;
-import org.neo4j.gds.GdsCypher.ModeBuildStage;
+import org.neo4j.gds.scaling.ScalarScaler;
 
 import java.util.Collection;
 import java.util.List;
@@ -119,6 +120,23 @@ void testWriteYields(ModeBuildStage queryBuilder, String testCaseName) {
         )));
     }
 
+    @ParameterizedTest(name = "{1}")
+    @MethodSource("org.neo4j.gds.pagerank.PageRankProcTest#graphVariations")
+    void shouldNotComputeCentralityDistributionOnLogScaler(ModeBuildStage queryBuilder, String testCaseName) {
+        var writeProp = "writeProp";
+        var query = queryBuilder
+            .writeMode()
+            .addParameter("scaler", ScalarScaler.Variant.LOG)
+            .addParameter("writeProperty", writeProp)
+            .yields("centralityDistribution");
+
+        assertCypherResult(query, List.of(
+            Map.of(
+                "centralityDistribution", Map.of("Error", "Unable to create histogram when using scaler of type LOG")
+            )
+        ));
+    }
+
     @Override
     public PageRankWriteConfig createConfig(CypherMapWrapper mapWrapper) {
         return PageRankWriteConfig.of(

Original file line number	Diff line number	Diff line change
`@@ -127,7 +127,7 @@ public DegreeCentralityMutateProc.MutateResult buildResult() {`
`127`	`127`	`computeMillis,`
`128`	`128`	`postProcessingMillis,`
`129`	`129`	`mutateMillis,`
`130`		`- centralityHistogramOrNull(),`
	`130`	`+ centralityHistogram,`
`131`	`131`	`config.toMap()`
`132`	`132`	`);`
`133`	`133`	`}`
Original file line number	Diff line number	Diff line change
`@@ -123,7 +123,7 @@ protected WriteResult buildResult() {`
`123`	`123`	`computeMillis,`
`124`	`124`	`postProcessingMillis,`
`125`	`125`	`writeMillis,`
`126`		`- centralityHistogramOrNull(),`
	`126`	`+ centralityHistogram,`
`127`	`127`	`config.toMap()`
`128`	`128`	`);`
`129`	`129`	`}`