From 3a105c3e3f8b22723e4a22ae95f73fc6712a044e Mon Sep 17 00:00:00 2001 From: James Brown <64858662+james-d-brown@users.noreply.github.com> Date: Fri, 23 Aug 2024 22:36:50 +0100 Subject: [PATCH] Consistently scrub threshold values from the summary statistics for geographic features or feature groups when a single conceptual threshold (e.g., "flood") has a threshold value that varies across features, #66. --- build.gradle | 3 +- src/wres/pipeline/EvaluationUtilities.java | 166 ++++++++++++------ src/wres/pipeline/Evaluator.java | 23 +-- .../pipeline/EvaluationUtilitiesTest.java | 113 +++++++++++- .../metrics/SummaryStatisticsCalculator.java | 7 + 5 files changed, 241 insertions(+), 71 deletions(-) diff --git a/build.gradle b/build.gradle index 5266a599ea..56524c4dab 100755 --- a/build.gradle +++ b/build.gradle @@ -2122,5 +2122,4 @@ configurations.runtimeOnly { exclude group: 'org.jboss.logmanager', module: 'jboss-logmanager' } -defaultTasks 'installDist', 'test', 'javadoc' - +defaultTasks 'installDist', 'test', 'javadoc' \ No newline at end of file diff --git a/src/wres/pipeline/EvaluationUtilities.java b/src/wres/pipeline/EvaluationUtilities.java index 06ccbed2f1..99cda957de 100644 --- a/src/wres/pipeline/EvaluationUtilities.java +++ b/src/wres/pipeline/EvaluationUtilities.java @@ -19,6 +19,7 @@ import java.util.function.Predicate; import java.util.function.Supplier; import java.util.function.ToIntFunction; +import java.util.function.UnaryOperator; import java.util.stream.Collectors; import java.util.stream.Stream; @@ -122,10 +123,6 @@ class EvaluationUtilities private static final String PERFORMING_RETRIEVAL_WITH_AN_IN_MEMORY_RETRIEVER_FACTORY = "Performing retrieval with an in-memory retriever factory."; - /** Metadata adapter for thresholds. */ - private static final BinaryOperator METADATA_ADAPTER_FOR_THRESHOLDS = - EvaluationUtilities.getMetadataAdapterForThresholds(); - /** Re-used string. */ private static final String PERFORMING_RETRIEVAL_WITH_A_RETRIEVER_FACTORY_BACKED_BY_A_PERSISTENT_STORE = "Performing retrieval with a retriever factory backed by a persistent store."; @@ -184,43 +181,21 @@ static void createAndPublishStatistics( EvaluationDetails evaluationDetails, } /** - * Creates and publishes the summary statistics. - * @param summaryStatistics the summary statistics calculators, mapped by message group identifier - * @param messager the evaluation messager - * @throws NullPointerException if any input is null + * Create and publish the summary statistics. + * + * @param evaluationDetails the evaluation details */ - static void createAndPublishSummaryStatistics( Map> summaryStatistics, - EvaluationMessager messager ) + static void createAndPublishSummaryStatistics( EvaluationDetails evaluationDetails ) { - Objects.requireNonNull( summaryStatistics ); - Objects.requireNonNull( messager ); - - LOGGER.debug( "Publishing summary statistics from {} summary statistics calculators.", - summaryStatistics.size() ); - - // Publish the summary statistics per message group - Set groupIds = new HashSet<>(); - for ( Map.Entry> next : summaryStatistics.entrySet() ) - { - // Generate the summary statistics - String groupId = next.getKey(); - List calculators = next.getValue(); - List nextStatistics = calculators.stream() - .flatMap( c -> c.get() - .stream() ) - .toList(); - - nextStatistics.forEach( m -> messager.publish( m, groupId ) ); - - groupIds.add( groupId ); - - LOGGER.debug( "Published {} summary statistics for group {}", nextStatistics.size(), groupId ); - } + Objects.requireNonNull( evaluationDetails ); - // Mark the publication complete for all groups - groupIds.forEach( messager::markGroupPublicationCompleteReportedSuccess ); + // Main dataset + EvaluationUtilities.createAndPublishSummaryStatistics( evaluationDetails.summaryStatistics(), + evaluationDetails.evaluation() ); - LOGGER.debug( "Finished publishing summary statistics." ); + // Baseline + EvaluationUtilities.createAndPublishSummaryStatistics( evaluationDetails.summaryStatisticsForBaseline(), + evaluationDetails.evaluation() ); } /** @@ -228,13 +203,15 @@ static void createAndPublishSummaryStatistics( Map> getSummaryStatisticsCalculators( EvaluationDeclaration declaration, - long poolCount ) + long poolCount, + boolean clearThresholdValues ) { Objects.requireNonNull( declaration ); @@ -248,6 +225,7 @@ static Map> getSummaryStatisticsCalcul } // Collect the geographic feature dimensions to aggregate, which are the only supported dimensions + // Note that clearing threshold values is linked to these aggregation dimensions. Set dimensions = declaration.summaryStatistics() .stream() @@ -256,7 +234,10 @@ static Map> getSummaryStatisticsCalcul || d == SummaryStatistic.StatisticDimension.FEATURES ) .collect( Collectors.toSet() ); - return EvaluationUtilities.getSummaryStatisticsForFeatures( declaration, dimensions, poolCount ); + return EvaluationUtilities.getSummaryStatisticsForFeatures( declaration, + dimensions, + poolCount, + clearThresholdValues ); } /** @@ -738,6 +719,74 @@ static Set getFeatureGroupsForSummaryStatisticsOnly( Set metricsAndThresholds ) + { + Objects.requireNonNull( metricsAndThresholds ); + + // Group the thresholds by name and determine whether any groups contain thresholds with different values + return metricsAndThresholds.stream() + .flatMap( s -> s.thresholds() + .values() + .stream() + .flatMap( Collection::stream ) ) + // Group the thresholds by name + .collect( Collectors.groupingBy( t -> t.getThreshold() + .getName() ) ) + // Are there any named thresholds with different threshold values? + .values() + .stream() + .map( t -> t.stream() + .map( ThresholdOuter::getValues ) + .collect( Collectors.toSet() ) ) + .anyMatch( c -> c.size() > 1 ); + } + + /** + * Creates and publishes the summary statistics. + * @param summaryStatistics the summary statistics calculators, mapped by message group identifier + * @param messager the evaluation messager + * @throws NullPointerException if any input is null + */ + private static void createAndPublishSummaryStatistics( Map> summaryStatistics, + EvaluationMessager messager ) + { + Objects.requireNonNull( summaryStatistics ); + Objects.requireNonNull( messager ); + + LOGGER.debug( "Publishing summary statistics from {} summary statistics calculators.", + summaryStatistics.size() ); + + // Publish the summary statistics per message group + Set groupIds = new HashSet<>(); + for ( Map.Entry> next : summaryStatistics.entrySet() ) + { + // Generate the summary statistics + String groupId = next.getKey(); + List calculators = next.getValue(); + List nextStatistics = calculators.stream() + .flatMap( c -> c.get() + .stream() ) + .toList(); + + nextStatistics.forEach( m -> messager.publish( m, groupId ) ); + + groupIds.add( groupId ); + + LOGGER.debug( "Published {} summary statistics for group {}", nextStatistics.size(), groupId ); + } + + // Mark the publication complete for all groups + groupIds.forEach( messager::markGroupPublicationCompleteReportedSuccess ); + + LOGGER.debug( "Finished publishing summary statistics." ); + } + /** * Creates one pool task for each pool request and then chains them together, such that all of the pools complete * nominally or one completes exceptionally. @@ -1518,13 +1567,15 @@ private static List> join( List> fir * @param dimensions the feature dimensions over which to perform aggregation * @return the summary statistics calculators * @param poolCount the number of pools for which raw (non-summary) statistics are required + * @param clearThresholdValues is true to clear event threshold values, false otherwise * @throws NullPointerException if any input is null * @throws IllegalArgumentException if the dimension is unsupported */ private static Map> getSummaryStatisticsForFeatures( EvaluationDeclaration declaration, Set dimensions, - long poolCount ) + long poolCount, + boolean clearThresholdValues ) { Objects.requireNonNull( declaration ); @@ -1538,7 +1589,8 @@ private static Map> getSummaryStatisti List timeWindowAndThresholdFilters = EvaluationUtilities.getTimeWindowAndThresholdFilters( timeWindows, thresholds, - separateMetricsForBaseline ); + separateMetricsForBaseline, + clearThresholdValues ); // Get the geographic feature filters and metadata adapters List featureFilters = new ArrayList<>(); @@ -1656,9 +1708,9 @@ else if ( behavioralName.isInGroup( MetricConstants.StatisticType.BOXPLOT_PER_PO BinaryOperator poolNumberAdapter = EvaluationUtilities.getMetadataAdapterForPoolNumber( poolNumber ); - BinaryOperator thresholdAdapter = nextInnerFilter.adapter(); + UnaryOperator thresholdAdapter = nextInnerFilter.adapter(); BinaryOperator metadataAdapter = ( p, q ) -> - poolNumberAdapter.apply( featureAdapter.apply( thresholdAdapter.apply( p, q ), q ), q ); + poolNumberAdapter.apply( featureAdapter.apply( thresholdAdapter.apply( p ), q ), q ); SummaryStatisticsCalculator calculator = SummaryStatisticsCalculator.of( nextScalar, nextDiagrams, @@ -1714,12 +1766,13 @@ private static BinaryOperator getMetadataAdapterForFeatureGroup( Geo /** * Creates a metadata adapter that removes threshold values if they are unequal across instances. * + * @param clearThresholds is true to clear event threshold values, false otherwise * @return the metadata adapter */ - private static BinaryOperator getMetadataAdapterForThresholds() + private static UnaryOperator getMetadataAdapterForThresholds( boolean clearThresholds ) { - return ( existing, latest ) -> + return existing -> { boolean isBaselinePool = !existing.hasPool() && existing.hasBaselinePool(); @@ -1729,15 +1782,11 @@ private static BinaryOperator getMetadataAdapterForThresholds() wres.statistics.generated.Pool.Builder existingPool = isBaselinePool ? adjusted.getBaselinePoolBuilder() : adjusted.getPoolBuilder(); - wres.statistics.generated.Pool latestPool = - isBaselinePool ? latest.getBaselinePool() : latest.getPool(); - - // Clear the threshold values unless they are equal across statistics + // Clear the threshold values unless they are equal across statistics or represent all data if ( existingPool.hasEventThreshold() - && !Objects.equals( existingPool.getEventThreshold() - .getLeftThresholdValue(), - latestPool.getEventThreshold() - .getLeftThresholdValue() ) ) + && !ThresholdOuter.of( existingPool.getEventThreshold() ) + .isAllDataThreshold() + && clearThresholds ) { // Set to missing rather than clearing: #126545 Threshold.Builder builder = existingPool.getEventThresholdBuilder() @@ -1975,17 +2024,22 @@ private static FeatureGroupFilterAdapter getBaselineFeatureGroupForSummaryStatis * @param timeWindows the time windows * @param thresholds the thresholds * @param separateMetricsForBaseline whether separate metrics are required for a baseline dataset + * @param clearThresholdValues is true to clear event threshold values, false otherwise * @return the filters */ private static List getTimeWindowAndThresholdFilters( Set timeWindows, Set thresholds, - boolean separateMetricsForBaseline ) + boolean separateMetricsForBaseline, + boolean clearThresholdValues ) { // Get the time window filters List> timeWindowFilters = EvaluationUtilities.getTimeWindowFilters( timeWindows, separateMetricsForBaseline ); + UnaryOperator adapter = + EvaluationUtilities.getMetadataAdapterForThresholds( clearThresholdValues ); + LOGGER.debug( "Discovered {} time windows, which produced {} filters.", timeWindows.size(), timeWindowFilters.size() ); @@ -2008,7 +2062,7 @@ private static List getTimeWindowAndThresho List nextFilters = joined.stream() .map( n -> new TimeWindowAndThresholdFilterAdapter( n, - METADATA_ADAPTER_FOR_THRESHOLDS, + adapter, nextCount ) ) .toList(); filters.addAll( nextFilters ); @@ -2045,7 +2099,7 @@ private record FeatureGroupFilterAdapter( GeometryGroup geometryGroup, * @param timeWindowNumber the time window number */ private record TimeWindowAndThresholdFilterAdapter( Predicate filter, - BinaryOperator adapter, + UnaryOperator adapter, long timeWindowNumber ) { } diff --git a/src/wres/pipeline/Evaluator.java b/src/wres/pipeline/Evaluator.java index ab35e03f66..f35deaa9bf 100644 --- a/src/wres/pipeline/Evaluator.java +++ b/src/wres/pipeline/Evaluator.java @@ -726,19 +726,26 @@ private Pair, String> evaluate( SystemSettings systemSettings, // this is feature-group shaped, but additional shapes may be desired in future PoolGroupTracker groupTracker = PoolGroupTracker.ofFeatureGroupTracker( evaluationMessager, poolRequests ); + // Are there event thresholds that vary across geographic features? If so, they should not cleared from + // any summary statistics that aggregate across geographic features + boolean clearThresholdValues = + EvaluationUtilities.hasEventThresholdsThatVaryAcrossFeatures( metricsAndThresholds ); + // Create the summary statistics calculators to increment with raw statistics Map> summaryStatsCalculators = EvaluationUtilities.getSummaryStatisticsCalculators( declarationWithFeaturesAndThresholds, - poolCount ); - Map> summaryStataCalculatorsForBaseline = Map.of(); + poolCount, + clearThresholdValues ); + Map> summaryStatsCalculatorsForBaseline = Map.of(); boolean separateMetricsForBaseline = DeclarationUtilities.hasBaseline( declaration ) && declaration.baseline() .separateMetrics(); if ( separateMetricsForBaseline ) { - summaryStataCalculatorsForBaseline = + summaryStatsCalculatorsForBaseline = EvaluationUtilities.getSummaryStatisticsCalculators( declarationWithFeaturesAndThresholds, - poolCount ); + poolCount, + clearThresholdValues ); } // Set the project and evaluation, metrics and thresholds and summary statistics @@ -749,7 +756,7 @@ private Pair, String> evaluate( SystemSettings systemSettings, .declaration( declarationWithFeaturesAndThresholds ) .metricsAndThresholds( metricsAndThresholds ) .summaryStatistics( summaryStatsCalculators ) - .summaryStatisticsForBaseline( summaryStataCalculatorsForBaseline ) + .summaryStatisticsForBaseline( summaryStatsCalculatorsForBaseline ) .summaryStatisticsOnly( doNotPublish ) .build(); @@ -761,11 +768,7 @@ private Pair, String> evaluate( SystemSettings systemSettings, executors ); // Create and publish any summary statistics derived from the raw statistics - EvaluationUtilities.createAndPublishSummaryStatistics( summaryStatsCalculators, - evaluationMessager ); - - EvaluationUtilities.createAndPublishSummaryStatistics( summaryStataCalculatorsForBaseline, - evaluationMessager ); + EvaluationUtilities.createAndPublishSummaryStatistics( evaluationDetails ); // Report that all publication was completed. At this stage, a message is sent indicating the expected // message count for all message types, thereby allowing consumers to know when all messages have arrived. diff --git a/test/wres/pipeline/EvaluationUtilitiesTest.java b/test/wres/pipeline/EvaluationUtilitiesTest.java index d84c7eb5aa..3e3153d241 100644 --- a/test/wres/pipeline/EvaluationUtilitiesTest.java +++ b/test/wres/pipeline/EvaluationUtilitiesTest.java @@ -1,6 +1,7 @@ package wres.pipeline; import java.util.Collection; +import java.util.HashMap; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; @@ -8,9 +9,12 @@ import org.junit.jupiter.api.Test; +import static org.junit.jupiter.api.Assertions.assertAll; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; +import wres.config.MetricConstants; import wres.config.yaml.DeclarationUtilities; import wres.config.yaml.components.EvaluationDeclaration; import wres.config.yaml.components.EvaluationDeclarationBuilder; @@ -19,6 +23,9 @@ import wres.config.yaml.components.ThresholdBuilder; import wres.config.yaml.components.ThresholdType; import wres.config.yaml.components.TimePools; +import wres.datamodel.space.FeatureTuple; +import wres.datamodel.thresholds.MetricsAndThresholds; +import wres.datamodel.thresholds.ThresholdOuter; import wres.metrics.SummaryStatisticsCalculator; import wres.statistics.generated.Geometry; import wres.statistics.generated.GeometryGroup; @@ -126,7 +133,7 @@ void testGetSummaryStatisticsCalculatorsWithTwoTimeWindowsAndTwoThresholdsAcross .build(); Map> calculators = - EvaluationUtilities.getSummaryStatisticsCalculators( evaluation, 0 ); + EvaluationUtilities.getSummaryStatisticsCalculators( evaluation, 0, false ); // Eight filters assertEquals( 8, calculators.values() @@ -170,7 +177,7 @@ void testGetSummaryStatisticsCalculatorsForNamedThresholdsAcrossAllFeatures() .build(); Map> calculators = - EvaluationUtilities.getSummaryStatisticsCalculators( evaluation, 0 ); + EvaluationUtilities.getSummaryStatisticsCalculators( evaluation, 0, false ); // One filter assertEquals( 1, calculators.size() ); @@ -271,7 +278,7 @@ void testGetSummaryStatisticsCalculatorsWithTwoTimeWindowsAcrossFeatureGroups() .build(); Map> calculators = - EvaluationUtilities.getSummaryStatisticsCalculators( evaluation, 0 ); + EvaluationUtilities.getSummaryStatisticsCalculators( evaluation, 0, false ); // Eight filters assertEquals( 4, calculators.values() @@ -279,4 +286,104 @@ void testGetSummaryStatisticsCalculatorsWithTwoTimeWindowsAcrossFeatureGroups() .mapToInt( List::size ) .sum() ); } + + @Test + void testHasEventThresholdsThatVaryAcrossFeaturesReturnsTrue() + { + Map> thresholds = new HashMap<>(); + + Threshold one = Threshold.newBuilder() + .setName( "flood" ) + .setLeftThresholdValue( 23.0 ) + .build(); + + Threshold two = Threshold.newBuilder() + .setName( "flood" ) + .setLeftThresholdValue( 22.0 ) + .build(); + + Geometry oneFeature = Geometry.newBuilder() + .setName( "foo" ) + .build(); + Geometry twoFeature = Geometry.newBuilder() + .setName( "bar" ) + .build(); + + FeatureTuple oneTuple = FeatureTuple.of( GeometryTuple.newBuilder() + .setLeft( oneFeature ) + .setRight( oneFeature ) + .build() ); + + FeatureTuple twoTuple = FeatureTuple.of( GeometryTuple.newBuilder() + .setLeft( twoFeature ) + .setRight( twoFeature ) + .build() ); + + thresholds.put( oneTuple, Set.of( ThresholdOuter.of( one ) ) ); + thresholds.put( twoTuple, Set.of( ThresholdOuter.of( two ) ) ); + + MetricsAndThresholds metricsAndThresholds = new MetricsAndThresholds( Set.of( MetricConstants.MEAN_ERROR ), + thresholds, + 0, + Pool.EnsembleAverageType.MEAN ); + + Set thresholdSet = Set.of( metricsAndThresholds ); + assertTrue( EvaluationUtilities.hasEventThresholdsThatVaryAcrossFeatures( thresholdSet ) ); + } + + @Test + void testHasEventThresholdsThatVaryAcrossFeaturesReturnsFalse() + { + Map> thresholds = new HashMap<>(); + + Threshold one = Threshold.newBuilder() + .setName( "flood" ) + .setLeftThresholdValue( 23.0 ) + .build(); + + Threshold two = Threshold.newBuilder() + .setName( "flood" ) + .setLeftThresholdValue( 23.0 ) + .build(); + + Geometry oneFeature = Geometry.newBuilder() + .setName( "foo" ) + .build(); + Geometry twoFeature = Geometry.newBuilder() + .setName( "bar" ) + .build(); + + FeatureTuple oneTuple = FeatureTuple.of( GeometryTuple.newBuilder() + .setLeft( oneFeature ) + .setRight( oneFeature ) + .build() ); + + FeatureTuple twoTuple = FeatureTuple.of( GeometryTuple.newBuilder() + .setLeft( twoFeature ) + .setRight( twoFeature ) + .build() ); + + thresholds.put( oneTuple, Set.of( ThresholdOuter.of( one ) ) ); + thresholds.put( twoTuple, Set.of( ThresholdOuter.of( two ) ) ); + + MetricsAndThresholds metricsAndThresholds = new MetricsAndThresholds( Set.of( MetricConstants.MEAN_ERROR ), + thresholds, + 0, + Pool.EnsembleAverageType.MEAN ); + + Set thresholdSet = Set.of( metricsAndThresholds ); + + Map> thresholdsTwo = new HashMap<>(); + thresholdsTwo.put( oneTuple, Set.of( ThresholdOuter.of( one ) ) ); + + MetricsAndThresholds metricsAndThresholdsTwo = new MetricsAndThresholds( Set.of( MetricConstants.MEAN_ERROR ), + thresholdsTwo, + 0, + Pool.EnsembleAverageType.MEAN ); + + Set thresholdSetTwo = Set.of( metricsAndThresholdsTwo ); + + assertAll( () -> assertFalse( EvaluationUtilities.hasEventThresholdsThatVaryAcrossFeatures( thresholdSet ) ), + () -> assertFalse( EvaluationUtilities.hasEventThresholdsThatVaryAcrossFeatures( thresholdSetTwo ) ) ); + } } \ No newline at end of file diff --git a/wres-metrics/src/wres/metrics/SummaryStatisticsCalculator.java b/wres-metrics/src/wres/metrics/SummaryStatisticsCalculator.java index 8eee89ecd9..2f63846647 100644 --- a/wres-metrics/src/wres/metrics/SummaryStatisticsCalculator.java +++ b/wres-metrics/src/wres/metrics/SummaryStatisticsCalculator.java @@ -251,6 +251,13 @@ private void updateStatisticsMetadata( Statistics latest ) .clearOneBoxPerPair() .clearOneBoxPerPool() .build(); + + // Apply metadata adaptation to the first instance in case any unconditional adaptations are requested, + // such as removing event threshold values. For example, event threshold values may be removed + // unconditionally, and this should occur even when the summary statistics are produced for a sample + // size of one + this.metadataAggregator.apply( this.nominal, this.nominal ); + LOGGER.debug( "Set the nominal statistics metadata for summary statistics calculator {} to: {}", this, this.nominal );