diff --git a/src/wres/pipeline/pooling/EventsGenerator.java b/src/wres/pipeline/pooling/EventsGenerator.java index 83d277760..97e519b4a 100644 --- a/src/wres/pipeline/pooling/EventsGenerator.java +++ b/src/wres/pipeline/pooling/EventsGenerator.java @@ -113,7 +113,7 @@ Set doEventDetection( Project project, Set events = new HashSet<>(); TimeScaleOuter desiredTimeScale = project.getDesiredTimeScale(); - boolean detectionAttempted = false; // Only attempt to combine events when detection has been attempted + int detectionAttemptedCount = 0; // Only attempt to combine events when detection has been attempted for ( EventDetectionDataset dataset : detection.datasets() ) { switch ( dataset ) @@ -195,8 +195,8 @@ Set doEventDetection( Project project, + "orientation." ); } - this.combineEvents( detectionAttempted, events, innerEvents, combination ); - detectionAttempted = true; + this.combineEvents( detectionAttemptedCount > 0, events, innerEvents, combination ); + detectionAttemptedCount++; } } case OBSERVED -> @@ -215,8 +215,8 @@ Set doEventDetection( Project project, this.measurementUnit(), declaration ); Set innerEvents = this.doEventDetection( details ); - this.combineEvents( detectionAttempted, events, innerEvents, combination ); - detectionAttempted = true; + this.combineEvents( detectionAttemptedCount > 0, events, innerEvents, combination ); + detectionAttemptedCount++; } case PREDICTED -> { @@ -234,8 +234,8 @@ Set doEventDetection( Project project, this.measurementUnit(), declaration ); Set innerEvents = this.doEventDetection( details ); - this.combineEvents( detectionAttempted, events, innerEvents, combination ); - detectionAttempted = true; + this.combineEvents( detectionAttemptedCount > 0, events, innerEvents, combination ); + detectionAttemptedCount++; } case BASELINE -> { @@ -253,8 +253,8 @@ Set doEventDetection( Project project, this.measurementUnit(), declaration ); Set innerEvents = this.doEventDetection( details ); - this.combineEvents( detectionAttempted, events, innerEvents, combination ); - detectionAttempted = true; + this.combineEvents( detectionAttemptedCount > 0, events, innerEvents, combination ); + detectionAttemptedCount++; } } } @@ -264,11 +264,15 @@ Set doEventDetection( Project project, featureGroup.getName(), combination ); - // Aggregate any intersecting events, as needed - Set aggregated = this.aggregateEvents( events, - detection.parameters() - .aggregation(), - featureGroup ); + // Aggregate any intersecting events, as needed, but only if combination/intersection happened + Set aggregated = events; + if ( detectionAttemptedCount > 1 ) // Combination/intersection happened + { + aggregated = this.aggregateEvents( events, + detection.parameters() + .aggregation(), + featureGroup ); + } // Adjust the time windows to account for existing time windows and other explicit time constraints Set declared = TimeWindowSlicer.getTimeWindows( declaration ); @@ -336,9 +340,9 @@ private Set doEventDetection( EventDetectionDetails details ) Stream> series = eventRetriever.getLeftRetriever( features, timeWindow ) .get(); - Set innerEvents = series.flatMap( s -> this.doEventDetection( s, - details, - this.leftUpscaler() ) + Set innerEvents = series.flatMap( s -> this.adjustTimeSeriesAndDetectEvents( s, + details, + this.leftUpscaler() ) .stream() ) .collect( Collectors.toSet() ); LOGGER.info( DETECTED_EVENTS_IN_THE_DATASET, @@ -352,9 +356,9 @@ private Set doEventDetection( EventDetectionDetails details ) Stream> series = eventRetriever.getRightRetriever( features, timeWindow ) .get(); - Set innerEvents = series.flatMap( s -> this.doEventDetection( s, - details, - this.rightUpscaler() ) + Set innerEvents = series.flatMap( s -> this.adjustTimeSeriesAndDetectEvents( s, + details, + this.rightUpscaler() ) .stream() ) .collect( Collectors.toSet() ); LOGGER.info( DETECTED_EVENTS_IN_THE_DATASET, @@ -368,9 +372,9 @@ private Set doEventDetection( EventDetectionDetails details ) Stream> series = eventRetriever.getBaselineRetriever( features, timeWindow ) .get(); - Set innerEvents = series.flatMap( s -> this.doEventDetection( s, - details, - this.baselineUpscaler() ) + Set innerEvents = series.flatMap( s -> this.adjustTimeSeriesAndDetectEvents( s, + details, + this.baselineUpscaler() ) .stream() ) .collect( Collectors.toSet() ); LOGGER.info( DETECTED_EVENTS_IN_THE_DATASET, @@ -405,10 +409,11 @@ private Set doEventDetection( EventDetectionDetails details ) }; Set innerEvents = series.map( counter ) - .flatMap( s -> this.doEventDetection( s, - this.getAdjustedDetails( details, s.getMetadata() - .getUnit() ), - this.covariateUpscaler() ) + .flatMap( s -> this.adjustTimeSeriesAndDetectEvents( s, + this.getAdjustedDetails( details, + s.getMetadata() + .getUnit() ), + this.covariateUpscaler() ) .stream() ) .collect( Collectors.toSet() ); LOGGER.info( "Detected {} events in the {} dataset containing {} time-series for feature group {} " @@ -603,16 +608,39 @@ private Set aggregateEvents( Set events, } /** - * Performs event detection. + * Performs rescaling and filtering of event values, followed by event detection. * * @param timeSeries the time-series * @param details the event detection details * @return the time windows, one for each detected event */ - private Set doEventDetection( TimeSeries timeSeries, - EventDetectionDetails details, - TimeSeriesUpscaler upscaler ) + private Set adjustTimeSeriesAndDetectEvents( TimeSeries timeSeries, + EventDetectionDetails details, + TimeSeriesUpscaler upscaler ) + { + // Rescale the time-series if needed + TimeSeries adjusted = this.getRescaledTimeSeries( timeSeries, details, upscaler ); + + LOGGER.debug( "Performing event detection of a time-series dataset containing {} events and the following " + + "metadata: {}.", adjusted.getEvents() + .size(), adjusted.getMetadata() ); + + return this.eventDetector() + .detect( adjusted ); + } + + /** + * Performs rescaling of the input time-series as needed. + * + * @param timeSeries the time-series + * @param details the event detection details + * @return the rescaled time-series + */ + + private TimeSeries getRescaledTimeSeries( TimeSeries timeSeries, + EventDetectionDetails details, + TimeSeriesUpscaler upscaler ) { // Upscale the time-series if needed boolean upscale = Objects.nonNull( details.desiredTimeScale() ) @@ -640,12 +668,7 @@ private Set doEventDetection( TimeSeries timeSeries, timeSeries = upscaled.getTimeSeries(); } - LOGGER.debug( "Performing event detection of a time-series dataset containing {} events and the following " - + "metadata: {}.", timeSeries.getEvents() - .size(), timeSeries.getMetadata() ); - - return this.eventDetector() - .detect( timeSeries ); + return timeSeries; } /** diff --git a/test/wres/pipeline/pooling/EventsGeneratorTest.java b/test/wres/pipeline/pooling/EventsGeneratorTest.java index 9708349ca..21c5998e5 100644 --- a/test/wres/pipeline/pooling/EventsGeneratorTest.java +++ b/test/wres/pipeline/pooling/EventsGeneratorTest.java @@ -734,6 +734,72 @@ void testEventDetectionAddsDeclaredTimeConstraintsToDetectedEvent() assertEquals( expected, actual ); } + @Test + void testEventDetectionIgnoresEventAggregationForSingleton() + { + // GitHub issue #420 + + TimeSeriesUpscaler upscaler = TimeSeriesOfDoubleUpscaler.of(); + EventDetectionParameters parameters = EventDetectionParametersBuilder.builder() + .windowSize( Duration.ofHours( 6 ) ) + .minimumEventDuration( Duration.ZERO ) + .halfLife( Duration.ofHours( 2 ) ) + .combination( EventDetectionCombination.INTERSECTION ) + .aggregation( TimeWindowAggregation.MAXIMUM ) + .build(); + EventDetector detector = EventDetectorFactory.getEventDetector( EventDetectionMethod.REGINA_OGDEN, + parameters ); + String measurementUnit = "qux"; + EventsGenerator generator = new EventsGenerator( upscaler, + upscaler, + upscaler, + upscaler, + measurementUnit, + detector ); + + TimeSeries timeSeriesOne = this.getTestTimeSeriesWithOffset( Duration.ZERO ); + + // Shift the series by one hour, which will eliminate the first event upon intersection, leaving two in total + // of the four events detected across the two series + TimeSeries timeSeriesTwo = this.getTestTimeSeriesWithOffset( Duration.ofHours( 1 ) ); + + // Mock a retriever factory + Mockito.when( this.leftRetriever.get() ) + .thenReturn( Stream.of( timeSeriesOne ) ); + Mockito.when( this.rightRetriever.get() ) + .thenReturn( Stream.of( timeSeriesTwo ) ); + Mockito.when( this.retrieverFactory.getLeftRetriever( Mockito.anySet(), Mockito.any() ) ) + .thenReturn( this.leftRetriever ); + Mockito.when( this.retrieverFactory.getRightRetriever( Mockito.anySet(), Mockito.any() ) ) + .thenReturn( this.rightRetriever ); + + // Mock the sufficient elements of a project with two separate datasets for event detection + EventDetection eventDeclaration = EventDetectionBuilder.builder() + .method( EventDetectionMethod.REGINA_OGDEN ) + .parameters( parameters ) + .datasets( Set.of( EventDetectionDataset.OBSERVED ) ) + .build(); + + EvaluationDeclaration declaration = EvaluationDeclarationBuilder.builder() + .eventDetection( eventDeclaration ) + .build(); + + Geometry geometry = MessageUtilities.getGeometry( "bar" ); + GeometryTuple geoTuple = MessageUtilities.getGeometryTuple( geometry, geometry, geometry ); + GeometryGroup geoGroup = MessageUtilities.getGeometryGroup( null, geoTuple ); + FeatureGroup groupOne = FeatureGroup.of( geoGroup ); + + Project project = Mockito.mock( Project.class ); + Mockito.when( project.getFeatureGroups() ) + .thenReturn( Set.of( groupOne ) ); + Mockito.when( project.getDeclaration() ) + .thenReturn( declaration ); + + Set actual = generator.doEventDetection( project, groupOne, this.retrieverFactory ); + + assertEquals( 2, actual.size() ); + } + /** * Generates a test time-series. * @param offset the offset to apply diff --git a/wres-config/src/wres/config/yaml/DeclarationValidator.java b/wres-config/src/wres/config/yaml/DeclarationValidator.java index 8b2da95ea..901534d35 100644 --- a/wres-config/src/wres/config/yaml/DeclarationValidator.java +++ b/wres-config/src/wres/config/yaml/DeclarationValidator.java @@ -38,6 +38,7 @@ import wres.config.yaml.components.Dataset; import wres.config.yaml.components.DatasetOrientation; import wres.config.yaml.components.EvaluationDeclaration; +import wres.config.yaml.components.EventDetection; import wres.config.yaml.components.EventDetectionCombination; import wres.config.yaml.components.EventDetectionDataset; import wres.config.yaml.components.EventDetectionParameters; @@ -2472,12 +2473,12 @@ private static List eventDetectionParametersAreValid( Eva { List events = new ArrayList<>(); + EventDetection detection = declaration.eventDetection(); + EventDetectionParameters parameters = detection.parameters(); + // Parameters undefined for which estimates/defaults are speculative: warn - if ( Objects.isNull( declaration.eventDetection() - .parameters() ) - || Objects.isNull( declaration.eventDetection() - .parameters() - .windowSize() ) ) + if ( Objects.isNull( parameters ) + || Objects.isNull( parameters.windowSize() ) ) { EvaluationStatusEvent warn = EvaluationStatusEvent.newBuilder() @@ -2491,11 +2492,8 @@ private static List eventDetectionParametersAreValid( Eva .build(); events.add( warn ); } - if ( Objects.isNull( declaration.eventDetection() - .parameters() ) - || Objects.isNull( declaration.eventDetection() - .parameters() - .halfLife() ) ) + if ( Objects.isNull( parameters ) + || Objects.isNull( parameters.halfLife() ) ) { EvaluationStatusEvent warn = EvaluationStatusEvent.newBuilder() @@ -2509,12 +2507,8 @@ private static List eventDetectionParametersAreValid( Eva .build(); events.add( warn ); } - if ( Objects.nonNull( declaration.eventDetection() - .parameters() ) ) + if ( Objects.nonNull( parameters ) ) { - EventDetectionParameters parameters = declaration.eventDetection() - .parameters(); - if ( parameters.combination() != EventDetectionCombination.INTERSECTION && Objects.nonNull( parameters.aggregation() ) ) { @@ -2538,6 +2532,29 @@ private static List eventDetectionParametersAreValid( Eva .build(); events.add( warn ); } + + // Warn if a non-default combination method is declared for a singleton: it will have no effect + if ( parameters.combination() != EventDetectionCombination.UNION + && detection.datasets() + .size() == 1 + && ( !detection.datasets() + .contains( EventDetectionDataset.COVARIATES ) + || declaration.covariates() + .size() == 1 ) ) + { + EvaluationStatusEvent warn + = EvaluationStatusEvent.newBuilder() + .setStatusLevel( StatusLevel.WARN ) + .setEventMessage( "Event detection was declared for a single dataset " + + "with 'combination' parameters, but these " + + "parameters are only applicable when performing " + + "event detection on more than one dataset. The " + + "'combination' parameters are redundant and will " + + "have no effect. For clarity, it would be better to " + + "remove them." ) + .build(); + events.add( warn ); + } } return Collections.unmodifiableList( events ); diff --git a/wres-config/test/wres/config/yaml/DeclarationValidatorTest.java b/wres-config/test/wres/config/yaml/DeclarationValidatorTest.java index 0a413a02b..ceeb65622 100644 --- a/wres-config/test/wres/config/yaml/DeclarationValidatorTest.java +++ b/wres-config/test/wres/config/yaml/DeclarationValidatorTest.java @@ -2855,6 +2855,33 @@ void testEventDetectionWithInconsistentParametersProducesError() StatusLevel.ERROR ) ); } + @Test + void testEventDetectionWithRedundantCombinationProducesWarning() + { + EventDetectionParameters parameters = + EventDetectionParametersBuilder.builder() + .combination( EventDetectionCombination.INTERSECTION ) + .build(); + EventDetection eventDetection = EventDetectionBuilder.builder() + .datasets( Set.of( EventDetectionDataset.OBSERVED ) ) + .parameters( parameters ) + .build(); + + EvaluationDeclaration declaration + = EvaluationDeclarationBuilder.builder() + .left( this.defaultDataset ) + .right( this.defaultDataset ) + .eventDetection( eventDetection ) + .build(); + + List events = DeclarationValidator.validate( declaration ); + + assertTrue( DeclarationValidatorTest.contains( events, + "these parameters are only applicable when performing " + + "event detection on more than one dataset", + StatusLevel.WARN ) ); + } + @Test void testInconsistentGraphicsOrientationAndPoolingDeclarationProducesError() throws IOException // NOSONAR {