Fix a bug in the interpolation of thresholds, #319.

NOAA-OWP · Sep 20, 2024 · 4a9dfed · 4a9dfed
1 parent 6d2045b
commit 4a9dfed
Show file tree

Hide file tree

Showing 12 changed files with 97 additions and 49 deletions.
diff --git a/wres-config/src/wres/config/yaml/DeclarationFactory.java b/wres-config/src/wres/config/yaml/DeclarationFactory.java
@@ -56,6 +56,7 @@
 import wres.config.yaml.components.DatasetOrientation;
 import wres.config.yaml.components.EvaluationDeclaration;
 import wres.config.yaml.components.Threshold;
+import wres.config.yaml.components.ThresholdBuilder;
 import wres.config.yaml.components.ThresholdOperator;
 import wres.config.yaml.components.ThresholdOrientation;
 import wres.config.yaml.components.ThresholdType;
@@ -131,7 +132,9 @@ public class DeclarationFactory
                                                  .setOperator( DEFAULT_THRESHOLD_OPERATOR.canonical() )
                                                  .build();
     /** Default wrapped threshold. */
-    public static final Threshold DEFAULT_THRESHOLD = new Threshold( DEFAULT_CANONICAL_THRESHOLD, null, null, null );
+    public static final Threshold DEFAULT_THRESHOLD =
+            ThresholdBuilder.builder().threshold( DEFAULT_CANONICAL_THRESHOLD )
+                            .build();
 
     /** To stringify protobufs into presentable JSON. */
     public static final Function<MessageOrBuilder, String> PROTBUF_STRINGIFIER = message -> {

diff --git a/wres-config/src/wres/config/yaml/DeclarationInterpolator.java b/wres-config/src/wres/config/yaml/DeclarationInterpolator.java
@@ -1676,6 +1676,11 @@ private static Metric addThresholdsToMetric( Map<wres.config.yaml.components.Thr
             // would render the request meaningless
             if ( parametersBuilder.thresholds()
                                   .size() == 1
+                 // Declared by a user, i.e., not generated by the software?
+                 && !parametersBuilder.thresholds()
+                                      .iterator()
+                                      .next()
+                                      .generated()
                  && parametersBuilder.thresholds()
                                      .iterator()
                                      .next()
@@ -1699,7 +1704,12 @@ private static Metric addThresholdsToMetric( Map<wres.config.yaml.components.Thr
         if ( name.isContinuous()
              && addAllData )
         {
-            valueThresholds.add( DeclarationUtilities.ALL_DATA_THRESHOLD );
+            // Flag this as an internally generated threshold
+            Threshold allData = ThresholdBuilder.builder( DeclarationUtilities.ALL_DATA_THRESHOLD )
+                                                .generated( true )
+                                                .build();
+
+            valueThresholds.add( allData );
         }
 
         // Render the value thresholds featureful, if possible

diff --git a/wres-config/src/wres/config/yaml/DeclarationMigrator.java b/wres-config/src/wres/config/yaml/DeclarationMigrator.java
@@ -2080,10 +2080,12 @@ private static Set<Threshold> migrateCsvThresholdsInline( ThresholdsConfig.Sourc
                                            .build();
                 Set<wres.statistics.generated.Threshold> toMigrate = nextEntry.getValue();
                 Set<Threshold> innerMigrated = toMigrate.stream()
-                                                        .map( next -> new Threshold( next,
-                                                                                     canonicalType,
-                                                                                     feature,
-                                                                                     featureNameFrom ) )
+                                                        .map( next -> ThresholdBuilder.builder()
+                                                                                      .threshold( next )
+                                                                                      .type( canonicalType )
+                                                                                      .feature( feature )
+                                                                                      .featureNameFrom( featureNameFrom )
+                                                                                      .build() )
                                                         .collect( Collectors.toCollection( LinkedHashSet::new ) );
                 migrated.addAll( innerMigrated );
             }

diff --git a/wres-config/src/wres/config/yaml/DeclarationUtilities.java b/wres-config/src/wres/config/yaml/DeclarationUtilities.java
@@ -62,6 +62,7 @@
 import wres.config.yaml.components.SourceBuilder;
 import wres.config.yaml.components.SourceInterface;
 import wres.config.yaml.components.Threshold;
+import wres.config.yaml.components.ThresholdBuilder;
 import wres.config.yaml.components.ThresholdType;
 import wres.config.yaml.components.TimeInterval;
 import wres.config.yaml.components.TimePools;
@@ -84,14 +85,19 @@ public class DeclarationUtilities
 
     /** All data threshold. */
     public static final Threshold ALL_DATA_THRESHOLD =
-            new Threshold( wres.statistics.generated.Threshold.newBuilder()
-                                                              .setLeftThresholdValue( Double.NEGATIVE_INFINITY )
-                                                              .setOperator( wres.statistics.generated.Threshold.ThresholdOperator.GREATER )
-                                                              .setDataType( wres.statistics.generated.Threshold.ThresholdDataType.LEFT_AND_RIGHT )
-                                                              .build(),
-                           wres.config.yaml.components.ThresholdType.VALUE,
-                           null,
-                           null );
+            ThresholdBuilder.builder()
+                            .threshold( wres.statistics.generated.Threshold.newBuilder()
+                                                                           .setLeftThresholdValue( Double.NEGATIVE_INFINITY )
+                                                                           .setOperator( wres.statistics.generated.Threshold.ThresholdOperator.GREATER )
+                                                                           .setDataType( wres.statistics.generated.Threshold.ThresholdDataType.LEFT_AND_RIGHT )
+                                                                           .build() )
+                            .type( wres.config.yaml.components.ThresholdType.VALUE )
+                            // All data threshold, as declared or read from a source, not generated by the software.
+                            // Distinguishing between an all data threshold as declared vs. generated internally is
+                            // important in some contexts, such as when establishing if the threshold was declared
+                            // explicitly as an override for a specific metric
+                            .generated( false )
+                            .build();
 
     /** Re-used message. */
     private static final String CANNOT_DETERMINE_TIME_WINDOWS_FROM_MISSING_DECLARATION = "Cannot determine time "

diff --git a/wres-config/src/wres/config/yaml/DeclarationValidator.java b/wres-config/src/wres/config/yaml/DeclarationValidator.java
@@ -3746,15 +3746,15 @@ private static List<EvaluationStatusEvent> validateFeaturefulThresholds( Set<Thr
         }
 
         // Explicit features and featureful thresholds. Some featureful thresholds must be correlated with features
-        Set<String> thresholdFeatureNames = thresholds.stream()
-                                                      .filter( n -> n.featureNameFrom() == orientation )
-                                                      // Ignore all data, which was added automagically
-                                                      .filter( n -> !DeclarationUtilities.ALL_DATA_THRESHOLD.threshold()
-                                                                                                            .equals(
-                                                                                                                    n.threshold() ) )
-                                                      .map( Threshold::feature )
-                                                      .map( wres.statistics.generated.Geometry::getName )
-                                                      .collect( Collectors.toSet() );
+        Set<String> thresholdFeatureNames =
+                thresholds.stream()
+                          .filter( n -> n.featureNameFrom() == orientation )
+                          // Ignore all data, which was added automagically
+                          .filter( n -> !DeclarationUtilities.ALL_DATA_THRESHOLD.threshold()
+                                                                                .equals( n.threshold() ) )
+                          .map( Threshold::feature )
+                          .map( wres.statistics.generated.Geometry::getName )
+                          .collect( Collectors.toSet() );
 
         if ( thresholdFeatureNames.isEmpty() )
         {

diff --git a/wres-config/src/wres/config/yaml/components/Threshold.java b/wres-config/src/wres/config/yaml/components/Threshold.java
@@ -17,12 +17,14 @@
  * @param type the threshold type to help identify the declaration context
  * @param feature a feature
  * @param featureNameFrom the orientation of the data from which the named feature is taken
+ * @param generated true if this threshold was generated by the software, false if declared or read from a source
  */
 @RecordBuilder
 public record Threshold( wres.statistics.generated.Threshold threshold,
                          ThresholdType type,
                          Geometry feature,
-                         DatasetOrientation featureNameFrom )
+                         DatasetOrientation featureNameFrom,
+                         boolean generated )
 {
     /** Logger. */
     private static final Logger LOGGER = LoggerFactory.getLogger( Threshold.class );
@@ -39,7 +41,8 @@ public record Threshold( wres.statistics.generated.Threshold threshold,
      */
     public Threshold
     {
-        if ( Objects.nonNull( feature ) && Objects.isNull( featureNameFrom ) )
+        if ( Objects.nonNull( feature )
+             && Objects.isNull( featureNameFrom ) )
         {
             LOGGER.debug( "Discovered a threshold for feature {}, but the orientation of the feature name was not "
                           + "supplied. Assuming an orientation of {}.",

diff --git a/wres-config/src/wres/config/yaml/deserializers/ThresholdsDeserializer.java b/wres-config/src/wres/config/yaml/deserializers/ThresholdsDeserializer.java
@@ -20,6 +20,7 @@
 import wres.config.yaml.DeclarationUtilities;
 import wres.config.yaml.components.DatasetOrientation;
 import wres.config.yaml.components.Threshold;
+import wres.config.yaml.components.ThresholdBuilder;
 import wres.config.yaml.components.ThresholdOrientation;
 import wres.config.yaml.components.ThresholdType;
 import wres.statistics.generated.Geometry;
@@ -178,7 +179,10 @@ private Set<Threshold> getSingletonThreshold( JsonNode thresholdNode,
         }
 
         wres.statistics.generated.Threshold nextThreshold = builder.build();
-        Threshold wrappedThreshold = new Threshold( nextThreshold, type, null, null );
+        Threshold wrappedThreshold = ThresholdBuilder.builder()
+                                                     .threshold( nextThreshold )
+                                                     .type( type )
+                                                     .build();
 
         return Set.of( wrappedThreshold );
     }
@@ -345,7 +349,12 @@ private Set<Threshold> getEmbellishedThresholds( ObjectReader reader,
             }
 
             Threshold nextThreshold =
-                    new Threshold( builder.build(), type, feature, featureNameFrom );
+                    ThresholdBuilder.builder()
+                                    .threshold( builder.build() )
+                                    .type( type )
+                                    .feature( feature )
+                                    .featureNameFrom( featureNameFrom )
+                                    .build();
             thresholds.add( nextThreshold );
         }
 
@@ -386,7 +395,10 @@ private Set<Threshold> getThresholdsFromArray( ObjectReader reader,
             }
 
             wres.statistics.generated.Threshold nextThreshold = thresholdBuilder.build();
-            Threshold nextWrappedThreshold = new Threshold( nextThreshold, type, null, null );
+            Threshold nextWrappedThreshold = ThresholdBuilder.builder()
+                                                             .threshold( nextThreshold )
+                                                             .type( type )
+                                                             .build();
             thresholds.add( nextWrappedThreshold );
         }
 

diff --git a/wres-config/src/wres/config/yaml/serializers/ThresholdSetsSerializer.java b/wres-config/src/wres/config/yaml/serializers/ThresholdSetsSerializer.java
@@ -13,6 +13,7 @@
 import org.slf4j.LoggerFactory;
 
 import wres.config.yaml.components.Threshold;
+import wres.config.yaml.components.ThresholdBuilder;
 import wres.config.yaml.components.ThresholdType;
 
 /**
@@ -37,7 +38,7 @@ public void serialize( Set<Threshold> thresholds,
 
         LOGGER.debug( "Discovered threshold sets with {} members.", grouped.size() );
 
-        if( !grouped.isEmpty() )
+        if ( !grouped.isEmpty() )
         {
             // Start the threshold sets
             writer.writeStartArray();
@@ -102,7 +103,12 @@ private Map<Threshold, Set<Threshold>> groupThresholdsByType( Set<Threshold> thr
                                                                 .clearLeftThresholdProbability()
                                                                 .build();
 
-            Threshold outer = new Threshold( nextInner, next.type(), next.feature(), next.featureNameFrom() );
+            Threshold outer = ThresholdBuilder.builder()
+                                              .threshold( nextInner )
+                                              .type( next.type() )
+                                              .feature( next.feature() )
+                                              .featureNameFrom( next.featureNameFrom() )
+                                              .build();
 
             if ( grouped.containsKey( outer ) )
             {

diff --git a/wres-config/src/wres/config/yaml/serializers/ThresholdsSerializer.java b/wres-config/src/wres/config/yaml/serializers/ThresholdsSerializer.java
@@ -19,6 +19,7 @@
 import wres.config.yaml.DeclarationFactory;
 import wres.config.yaml.DeclarationUtilities;
 import wres.config.yaml.components.Threshold;
+import wres.config.yaml.components.ThresholdBuilder;
 import wres.config.yaml.components.ThresholdOrientation;
 import wres.config.yaml.components.ThresholdType;
 import wres.statistics.generated.Geometry;
@@ -329,7 +330,9 @@ private boolean hasDefaultMetadata( Set<Threshold> thresholds )
                                                             .clearRightThresholdValue()
                                                             .build();
 
-            Threshold blankOuter = new Threshold( blank, null, null, null );
+            Threshold blankOuter = ThresholdBuilder.builder()
+                                                   .threshold( blank )
+                                                   .build();
 
             if ( !blankOuter.equals( DeclarationFactory.DEFAULT_THRESHOLD )
                  || Objects.nonNull( next.feature() ) )

diff --git a/wres-config/test/wres/config/yaml/DeclarationInterpolatorTest.java b/wres-config/test/wres/config/yaml/DeclarationInterpolatorTest.java
@@ -162,13 +162,15 @@ class DeclarationInterpolatorTest
     /** All data threshold. */
     private static final wres.config.yaml.components.Threshold
             ALL_DATA_THRESHOLD =
-            new wres.config.yaml.components.Threshold( Threshold.newBuilder()
-                                                                .setLeftThresholdValue( Double.NEGATIVE_INFINITY )
-                                                                .setOperator( Threshold.ThresholdOperator.GREATER )
-                                                                .setDataType( Threshold.ThresholdDataType.LEFT_AND_RIGHT )
-                                                                .build(),
-                                                       ThresholdType.VALUE,
-                                                       null, null );
+            wres.config.yaml.components.ThresholdBuilder.builder()
+                                                        .threshold( Threshold.newBuilder()
+                                                                             .setLeftThresholdValue( Double.NEGATIVE_INFINITY )
+                                                                             .setOperator( Threshold.ThresholdOperator.GREATER )
+                                                                             .setDataType( Threshold.ThresholdDataType.LEFT_AND_RIGHT )
+                                                                             .build() )
+                                                        .type( ThresholdType.VALUE )
+                                                        .generated( true )
+                                                        .build();
     /** Default list of observed sources in the old-style declaration. */
     List<DataSourceConfig.Source> observedSources;
     /** Default list of predicted sources in the old-style declaration. */
@@ -1904,22 +1906,22 @@ void testDeserializeAndInterpolateAddsUnitTothresholds() throws IOException
 
         assertAll( () -> assertTrue( actualInterpolated.thresholds()
                                                        .stream()
-                                                       .filter( next -> next
-                                                                        != DeclarationUtilities.ALL_DATA_THRESHOLD )
+                                                       .filter( next -> next.threshold()
+                                                                        != DeclarationUtilities.ALL_DATA_THRESHOLD.threshold() )
                                                        .allMatch( next -> "foo".equals( next.threshold()
                                                                                             .getThresholdValueUnits() ) ) ),
                    () -> assertTrue( actualInterpolated.thresholdSets()
                                                        .stream()
-                                                       .filter( next -> next
-                                                                        != DeclarationUtilities.ALL_DATA_THRESHOLD )
+                                                       .filter( next -> next.threshold()
+                                                                        != DeclarationUtilities.ALL_DATA_THRESHOLD.threshold() )
                                                        .allMatch( next -> "foo".equals( next.threshold()
                                                                                             .getThresholdValueUnits() ) ) ),
                    () -> assertTrue( actualInterpolated.metrics()
                                                        .stream()
                                                        .map( Metric::parameters )
                                                        .flatMap( next -> next.thresholds().stream() )
-                                                       .filter( next -> next
-                                                                        != DeclarationUtilities.ALL_DATA_THRESHOLD )
+                                                       .filter( next -> next.threshold()
+                                                                        != DeclarationUtilities.ALL_DATA_THRESHOLD.threshold() )
                                                        .allMatch( next -> "foo".equals( next.threshold()
                                                                                             .getThresholdValueUnits() ) ) )
         );

diff --git a/wres-config/test/wres/config/yaml/DeclarationValidatorTest.java b/wres-config/test/wres/config/yaml/DeclarationValidatorTest.java
@@ -1752,10 +1752,10 @@ void testRealValuedThresholdWithoutUnitProducesWarning()
                                        .build();
 
         wres.config.yaml.components.Threshold wrapped =
-                new wres.config.yaml.components.Threshold( threshold,
-                                                           ThresholdType.VALUE,
-                                                           null,
-                                                           null );
+                wres.config.yaml.components.ThresholdBuilder.builder()
+                                                            .threshold( threshold )
+                                                            .type( ThresholdType.VALUE )
+                                                            .build();
 
         EvaluationDeclaration declaration =
                 EvaluationDeclarationBuilder.builder()

diff --git a/wres-reading/src/wres/reading/wrds/thresholds/WrdsThresholdReader.java b/wres-reading/src/wres/reading/wrds/thresholds/WrdsThresholdReader.java
@@ -618,7 +618,8 @@ else if ( LOGGER.isDebugEnabled() )
             }
         }
 
-        if ( LOGGER.isDebugEnabled() && !featuresNotRequired.isEmpty() )
+        if ( LOGGER.isDebugEnabled()
+             && !featuresNotRequired.isEmpty() )
         {
             LOGGER.debug( "Thresholds were discovered for the following features whose thresholds were not "
                           + "required: {}",