Skip to content

Commit 4a9dfed

Browse files
james-d-brownjames-d-brown
committed
Fix a bug in the interpolation of thresholds, #319.
1 parent 6d2045b commit 4a9dfed

12 files changed

+97
-49
lines changed

Diff for: ‎wres-config/src/wres/config/yaml/DeclarationFactory.java

+4-1
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@
5656
import wres.config.yaml.components.DatasetOrientation;
5757
import wres.config.yaml.components.EvaluationDeclaration;
5858
import wres.config.yaml.components.Threshold;
59+
import wres.config.yaml.components.ThresholdBuilder;
5960
import wres.config.yaml.components.ThresholdOperator;
6061
import wres.config.yaml.components.ThresholdOrientation;
6162
import wres.config.yaml.components.ThresholdType;
@@ -131,7 +132,9 @@ public class DeclarationFactory
131132
.setOperator( DEFAULT_THRESHOLD_OPERATOR.canonical() )
132133
.build();
133134
/** Default wrapped threshold. */
134-
public static final Threshold DEFAULT_THRESHOLD = new Threshold( DEFAULT_CANONICAL_THRESHOLD, null, null, null );
135+
public static final Threshold DEFAULT_THRESHOLD =
136+
ThresholdBuilder.builder().threshold( DEFAULT_CANONICAL_THRESHOLD )
137+
.build();
135138

136139
/** To stringify protobufs into presentable JSON. */
137140
public static final Function<MessageOrBuilder, String> PROTBUF_STRINGIFIER = message -> {

Diff for: ‎wres-config/src/wres/config/yaml/DeclarationInterpolator.java

+11-1
Original file line numberDiff line numberDiff line change
@@ -1676,6 +1676,11 @@ private static Metric addThresholdsToMetric( Map<wres.config.yaml.components.Thr
16761676
// would render the request meaningless
16771677
if ( parametersBuilder.thresholds()
16781678
.size() == 1
1679+
// Declared by a user, i.e., not generated by the software?
1680+
&& !parametersBuilder.thresholds()
1681+
.iterator()
1682+
.next()
1683+
.generated()
16791684
&& parametersBuilder.thresholds()
16801685
.iterator()
16811686
.next()
@@ -1699,7 +1704,12 @@ private static Metric addThresholdsToMetric( Map<wres.config.yaml.components.Thr
16991704
if ( name.isContinuous()
17001705
&& addAllData )
17011706
{
1702-
valueThresholds.add( DeclarationUtilities.ALL_DATA_THRESHOLD );
1707+
// Flag this as an internally generated threshold
1708+
Threshold allData = ThresholdBuilder.builder( DeclarationUtilities.ALL_DATA_THRESHOLD )
1709+
.generated( true )
1710+
.build();
1711+
1712+
valueThresholds.add( allData );
17031713
}
17041714

17051715
// Render the value thresholds featureful, if possible

Diff for: ‎wres-config/src/wres/config/yaml/DeclarationMigrator.java

+6-4
Original file line numberDiff line numberDiff line change
@@ -2080,10 +2080,12 @@ private static Set<Threshold> migrateCsvThresholdsInline( ThresholdsConfig.Sourc
20802080
.build();
20812081
Set<wres.statistics.generated.Threshold> toMigrate = nextEntry.getValue();
20822082
Set<Threshold> innerMigrated = toMigrate.stream()
2083-
.map( next -> new Threshold( next,
2084-
canonicalType,
2085-
feature,
2086-
featureNameFrom ) )
2083+
.map( next -> ThresholdBuilder.builder()
2084+
.threshold( next )
2085+
.type( canonicalType )
2086+
.feature( feature )
2087+
.featureNameFrom( featureNameFrom )
2088+
.build() )
20872089
.collect( Collectors.toCollection( LinkedHashSet::new ) );
20882090
migrated.addAll( innerMigrated );
20892091
}

Diff for: ‎wres-config/src/wres/config/yaml/DeclarationUtilities.java

+14-8
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@
6262
import wres.config.yaml.components.SourceBuilder;
6363
import wres.config.yaml.components.SourceInterface;
6464
import wres.config.yaml.components.Threshold;
65+
import wres.config.yaml.components.ThresholdBuilder;
6566
import wres.config.yaml.components.ThresholdType;
6667
import wres.config.yaml.components.TimeInterval;
6768
import wres.config.yaml.components.TimePools;
@@ -84,14 +85,19 @@ public class DeclarationUtilities
8485

8586
/** All data threshold. */
8687
public static final Threshold ALL_DATA_THRESHOLD =
87-
new Threshold( wres.statistics.generated.Threshold.newBuilder()
88-
.setLeftThresholdValue( Double.NEGATIVE_INFINITY )
89-
.setOperator( wres.statistics.generated.Threshold.ThresholdOperator.GREATER )
90-
.setDataType( wres.statistics.generated.Threshold.ThresholdDataType.LEFT_AND_RIGHT )
91-
.build(),
92-
wres.config.yaml.components.ThresholdType.VALUE,
93-
null,
94-
null );
88+
ThresholdBuilder.builder()
89+
.threshold( wres.statistics.generated.Threshold.newBuilder()
90+
.setLeftThresholdValue( Double.NEGATIVE_INFINITY )
91+
.setOperator( wres.statistics.generated.Threshold.ThresholdOperator.GREATER )
92+
.setDataType( wres.statistics.generated.Threshold.ThresholdDataType.LEFT_AND_RIGHT )
93+
.build() )
94+
.type( wres.config.yaml.components.ThresholdType.VALUE )
95+
// All data threshold, as declared or read from a source, not generated by the software.
96+
// Distinguishing between an all data threshold as declared vs. generated internally is
97+
// important in some contexts, such as when establishing if the threshold was declared
98+
// explicitly as an override for a specific metric
99+
.generated( false )
100+
.build();
95101

96102
/** Re-used message. */
97103
private static final String CANNOT_DETERMINE_TIME_WINDOWS_FROM_MISSING_DECLARATION = "Cannot determine time "

Diff for: ‎wres-config/src/wres/config/yaml/DeclarationValidator.java

+9-9
Original file line numberDiff line numberDiff line change
@@ -3746,15 +3746,15 @@ private static List<EvaluationStatusEvent> validateFeaturefulThresholds( Set<Thr
37463746
}
37473747

37483748
// Explicit features and featureful thresholds. Some featureful thresholds must be correlated with features
3749-
Set<String> thresholdFeatureNames = thresholds.stream()
3750-
.filter( n -> n.featureNameFrom() == orientation )
3751-
// Ignore all data, which was added automagically
3752-
.filter( n -> !DeclarationUtilities.ALL_DATA_THRESHOLD.threshold()
3753-
.equals(
3754-
n.threshold() ) )
3755-
.map( Threshold::feature )
3756-
.map( wres.statistics.generated.Geometry::getName )
3757-
.collect( Collectors.toSet() );
3749+
Set<String> thresholdFeatureNames =
3750+
thresholds.stream()
3751+
.filter( n -> n.featureNameFrom() == orientation )
3752+
// Ignore all data, which was added automagically
3753+
.filter( n -> !DeclarationUtilities.ALL_DATA_THRESHOLD.threshold()
3754+
.equals( n.threshold() ) )
3755+
.map( Threshold::feature )
3756+
.map( wres.statistics.generated.Geometry::getName )
3757+
.collect( Collectors.toSet() );
37583758

37593759
if ( thresholdFeatureNames.isEmpty() )
37603760
{

Diff for: ‎wres-config/src/wres/config/yaml/components/Threshold.java

+5-2
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,14 @@
1717
* @param type the threshold type to help identify the declaration context
1818
* @param feature a feature
1919
* @param featureNameFrom the orientation of the data from which the named feature is taken
20+
* @param generated true if this threshold was generated by the software, false if declared or read from a source
2021
*/
2122
@RecordBuilder
2223
public record Threshold( wres.statistics.generated.Threshold threshold,
2324
ThresholdType type,
2425
Geometry feature,
25-
DatasetOrientation featureNameFrom )
26+
DatasetOrientation featureNameFrom,
27+
boolean generated )
2628
{
2729
/** Logger. */
2830
private static final Logger LOGGER = LoggerFactory.getLogger( Threshold.class );
@@ -39,7 +41,8 @@ public record Threshold( wres.statistics.generated.Threshold threshold,
3941
*/
4042
public Threshold
4143
{
42-
if ( Objects.nonNull( feature ) && Objects.isNull( featureNameFrom ) )
44+
if ( Objects.nonNull( feature )
45+
&& Objects.isNull( featureNameFrom ) )
4346
{
4447
LOGGER.debug( "Discovered a threshold for feature {}, but the orientation of the feature name was not "
4548
+ "supplied. Assuming an orientation of {}.",

Diff for: ‎wres-config/src/wres/config/yaml/deserializers/ThresholdsDeserializer.java

+15-3
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import wres.config.yaml.DeclarationUtilities;
2121
import wres.config.yaml.components.DatasetOrientation;
2222
import wres.config.yaml.components.Threshold;
23+
import wres.config.yaml.components.ThresholdBuilder;
2324
import wres.config.yaml.components.ThresholdOrientation;
2425
import wres.config.yaml.components.ThresholdType;
2526
import wres.statistics.generated.Geometry;
@@ -178,7 +179,10 @@ private Set<Threshold> getSingletonThreshold( JsonNode thresholdNode,
178179
}
179180

180181
wres.statistics.generated.Threshold nextThreshold = builder.build();
181-
Threshold wrappedThreshold = new Threshold( nextThreshold, type, null, null );
182+
Threshold wrappedThreshold = ThresholdBuilder.builder()
183+
.threshold( nextThreshold )
184+
.type( type )
185+
.build();
182186

183187
return Set.of( wrappedThreshold );
184188
}
@@ -345,7 +349,12 @@ private Set<Threshold> getEmbellishedThresholds( ObjectReader reader,
345349
}
346350

347351
Threshold nextThreshold =
348-
new Threshold( builder.build(), type, feature, featureNameFrom );
352+
ThresholdBuilder.builder()
353+
.threshold( builder.build() )
354+
.type( type )
355+
.feature( feature )
356+
.featureNameFrom( featureNameFrom )
357+
.build();
349358
thresholds.add( nextThreshold );
350359
}
351360

@@ -386,7 +395,10 @@ private Set<Threshold> getThresholdsFromArray( ObjectReader reader,
386395
}
387396

388397
wres.statistics.generated.Threshold nextThreshold = thresholdBuilder.build();
389-
Threshold nextWrappedThreshold = new Threshold( nextThreshold, type, null, null );
398+
Threshold nextWrappedThreshold = ThresholdBuilder.builder()
399+
.threshold( nextThreshold )
400+
.type( type )
401+
.build();
390402
thresholds.add( nextWrappedThreshold );
391403
}
392404

Diff for: ‎wres-config/src/wres/config/yaml/serializers/ThresholdSetsSerializer.java

+8-2
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
import org.slf4j.LoggerFactory;
1414

1515
import wres.config.yaml.components.Threshold;
16+
import wres.config.yaml.components.ThresholdBuilder;
1617
import wres.config.yaml.components.ThresholdType;
1718

1819
/**
@@ -37,7 +38,7 @@ public void serialize( Set<Threshold> thresholds,
3738

3839
LOGGER.debug( "Discovered threshold sets with {} members.", grouped.size() );
3940

40-
if( !grouped.isEmpty() )
41+
if ( !grouped.isEmpty() )
4142
{
4243
// Start the threshold sets
4344
writer.writeStartArray();
@@ -102,7 +103,12 @@ private Map<Threshold, Set<Threshold>> groupThresholdsByType( Set<Threshold> thr
102103
.clearLeftThresholdProbability()
103104
.build();
104105

105-
Threshold outer = new Threshold( nextInner, next.type(), next.feature(), next.featureNameFrom() );
106+
Threshold outer = ThresholdBuilder.builder()
107+
.threshold( nextInner )
108+
.type( next.type() )
109+
.feature( next.feature() )
110+
.featureNameFrom( next.featureNameFrom() )
111+
.build();
106112

107113
if ( grouped.containsKey( outer ) )
108114
{

Diff for: ‎wres-config/src/wres/config/yaml/serializers/ThresholdsSerializer.java

+4-1
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import wres.config.yaml.DeclarationFactory;
2020
import wres.config.yaml.DeclarationUtilities;
2121
import wres.config.yaml.components.Threshold;
22+
import wres.config.yaml.components.ThresholdBuilder;
2223
import wres.config.yaml.components.ThresholdOrientation;
2324
import wres.config.yaml.components.ThresholdType;
2425
import wres.statistics.generated.Geometry;
@@ -329,7 +330,9 @@ private boolean hasDefaultMetadata( Set<Threshold> thresholds )
329330
.clearRightThresholdValue()
330331
.build();
331332

332-
Threshold blankOuter = new Threshold( blank, null, null, null );
333+
Threshold blankOuter = ThresholdBuilder.builder()
334+
.threshold( blank )
335+
.build();
333336

334337
if ( !blankOuter.equals( DeclarationFactory.DEFAULT_THRESHOLD )
335338
|| Objects.nonNull( next.feature() ) )

Diff for: ‎wres-config/test/wres/config/yaml/DeclarationInterpolatorTest.java

+15-13
Original file line numberDiff line numberDiff line change
@@ -162,13 +162,15 @@ class DeclarationInterpolatorTest
162162
/** All data threshold. */
163163
private static final wres.config.yaml.components.Threshold
164164
ALL_DATA_THRESHOLD =
165-
new wres.config.yaml.components.Threshold( Threshold.newBuilder()
166-
.setLeftThresholdValue( Double.NEGATIVE_INFINITY )
167-
.setOperator( Threshold.ThresholdOperator.GREATER )
168-
.setDataType( Threshold.ThresholdDataType.LEFT_AND_RIGHT )
169-
.build(),
170-
ThresholdType.VALUE,
171-
null, null );
165+
wres.config.yaml.components.ThresholdBuilder.builder()
166+
.threshold( Threshold.newBuilder()
167+
.setLeftThresholdValue( Double.NEGATIVE_INFINITY )
168+
.setOperator( Threshold.ThresholdOperator.GREATER )
169+
.setDataType( Threshold.ThresholdDataType.LEFT_AND_RIGHT )
170+
.build() )
171+
.type( ThresholdType.VALUE )
172+
.generated( true )
173+
.build();
172174
/** Default list of observed sources in the old-style declaration. */
173175
List<DataSourceConfig.Source> observedSources;
174176
/** Default list of predicted sources in the old-style declaration. */
@@ -1904,22 +1906,22 @@ void testDeserializeAndInterpolateAddsUnitTothresholds() throws IOException
19041906

19051907
assertAll( () -> assertTrue( actualInterpolated.thresholds()
19061908
.stream()
1907-
.filter( next -> next
1908-
!= DeclarationUtilities.ALL_DATA_THRESHOLD )
1909+
.filter( next -> next.threshold()
1910+
!= DeclarationUtilities.ALL_DATA_THRESHOLD.threshold() )
19091911
.allMatch( next -> "foo".equals( next.threshold()
19101912
.getThresholdValueUnits() ) ) ),
19111913
() -> assertTrue( actualInterpolated.thresholdSets()
19121914
.stream()
1913-
.filter( next -> next
1914-
!= DeclarationUtilities.ALL_DATA_THRESHOLD )
1915+
.filter( next -> next.threshold()
1916+
!= DeclarationUtilities.ALL_DATA_THRESHOLD.threshold() )
19151917
.allMatch( next -> "foo".equals( next.threshold()
19161918
.getThresholdValueUnits() ) ) ),
19171919
() -> assertTrue( actualInterpolated.metrics()
19181920
.stream()
19191921
.map( Metric::parameters )
19201922
.flatMap( next -> next.thresholds().stream() )
1921-
.filter( next -> next
1922-
!= DeclarationUtilities.ALL_DATA_THRESHOLD )
1923+
.filter( next -> next.threshold()
1924+
!= DeclarationUtilities.ALL_DATA_THRESHOLD.threshold() )
19231925
.allMatch( next -> "foo".equals( next.threshold()
19241926
.getThresholdValueUnits() ) ) )
19251927
);

Diff for: ‎wres-config/test/wres/config/yaml/DeclarationValidatorTest.java

+4-4
Original file line numberDiff line numberDiff line change
@@ -1752,10 +1752,10 @@ void testRealValuedThresholdWithoutUnitProducesWarning()
17521752
.build();
17531753

17541754
wres.config.yaml.components.Threshold wrapped =
1755-
new wres.config.yaml.components.Threshold( threshold,
1756-
ThresholdType.VALUE,
1757-
null,
1758-
null );
1755+
wres.config.yaml.components.ThresholdBuilder.builder()
1756+
.threshold( threshold )
1757+
.type( ThresholdType.VALUE )
1758+
.build();
17591759

17601760
EvaluationDeclaration declaration =
17611761
EvaluationDeclarationBuilder.builder()

Diff for: ‎wres-reading/src/wres/reading/wrds/thresholds/WrdsThresholdReader.java

+2-1
Original file line numberDiff line numberDiff line change
@@ -618,7 +618,8 @@ else if ( LOGGER.isDebugEnabled() )
618618
}
619619
}
620620

621-
if ( LOGGER.isDebugEnabled() && !featuresNotRequired.isEmpty() )
621+
if ( LOGGER.isDebugEnabled()
622+
&& !featuresNotRequired.isEmpty() )
622623
{
623624
LOGGER.debug( "Thresholds were discovered for the following features whose thresholds were not "
624625
+ "required: {}",

0 commit comments

Comments
 (0)