-
Notifications
You must be signed in to change notification settings - Fork 6
Handling no received data from WeatherSource #1145
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
This reverts commit 529bf23
src/main/java/edu/ie3/datamodel/exceptions/NoWeatherDataException.java
Outdated
Show resolved
Hide resolved
…d changed NoDataException
Handling CSV-Source done
|
@danielfeismann |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for including my points. I do have another one or two points which are a bit unclear to me
@@ -75,6 +75,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
### Added | |||
- Enhance `TimeSeriesSource` with method to retrieve all time keys after a given key [#543](https://github.com/ie3-institute/PowerSystemDataModel/issues/543) | |||
- Enhance `WeatherSource` with method to retrieve all time keys after a given key [#572](https://github.com/ie3-institute/PowerSystemDataModel/issues/572) | |||
- Added explicit handling for cases where no weather data is received from any source [#554](https://github.com/ie3-institute/PowerSystemDataModel/issues/554) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move this to the end of this subsection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder to move this to the Unreleased/Snapshot
section
src/main/java/edu/ie3/datamodel/exceptions/NoDataException.java
Outdated
Show resolved
Hide resolved
src/test/groovy/edu/ie3/datamodel/io/source/influxdb/InfluxDbWeatherSourceIconIT.groovy
Outdated
Show resolved
Hide resolved
src/test/groovy/edu/ie3/datamodel/io/source/influxdb/InfluxDbWeatherSourceCosmoIT.groovy
Show resolved
Hide resolved
IndividualTimeSeries<WeatherValue> timeSeries = | ||
new IndividualTimeSeries<>(timeBasedValues); | ||
coordinateToTimeSeries.put(entry.getKey(), timeSeries); | ||
if (!timeBasedValues.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it also lead to an exception in case timeBasedValues is empty, since one could assume we would expect weather data when asking for it and those aren't there? Not sure about this, I might have to rethink on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, testing this would also be great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested throwing an exception when timeBasedValues is empty, but that broke existing behavior and tests. For now we keep it simple: invalid coordinates throw a NoDataException, but valid coordinates with no data just return an empty series. Or do we really want it that strict? If so we could use my last commit which I reverted again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be great to have equivalent behavior across types of data sources, at least where it's possible
src/main/java/edu/ie3/datamodel/exceptions/NoDataException.java
Outdated
Show resolved
Hide resolved
src/main/java/edu/ie3/datamodel/io/source/couchbase/CouchbaseWeatherSource.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While looking through this, it came to my mind that some of these errors we could try and heal instead, before throwing an exception.
I realize this requires a bit more effort, but maybe it still makes sense to handle it in this PR? On the other hand, SIMONA has to deal with these new types of exceptions anyway, and at a later step it would crash less because we healed the problem instead.
@@ -75,6 +75,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
### Added | |||
- Enhance `TimeSeriesSource` with method to retrieve all time keys after a given key [#543](https://github.com/ie3-institute/PowerSystemDataModel/issues/543) | |||
- Enhance `WeatherSource` with method to retrieve all time keys after a given key [#572](https://github.com/ie3-institute/PowerSystemDataModel/issues/572) | |||
- Added explicit handling for cases where no weather data is received from any source [#554](https://github.com/ie3-institute/PowerSystemDataModel/issues/554) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder to move this to the Unreleased/Snapshot
section
logger.error("Unable to match coordinate {} to a coordinate ID", coordinate); | ||
throw new NoDataException("No coordinate ID found for the given point: " + coordinate); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logging an error is obsolete when we also throw and exception in the next line, imho
if (!invalidCoordinates.isEmpty()) { | ||
throw new NoDataException("No data for given coordinates: " + invalidCoordinates); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I think there's a way to heal this instead of breaking right away.
I'd say, if for a coordinate we cannot retrieve weather data, we try to provide the last available data (the available data for the last point in time before now) first. In this case, we also log a warning.
If that also fails though, we should throw an exception.
if (!invalidCoordinates.isEmpty()) { | ||
throw new NoDataException("No data for given coordinates: " + invalidCoordinates); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
new NoDataException( | ||
"No valid weather data found for the given date and coordinate.")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here: Maybe try to take an earlier data point instead?
new NoDataException( | ||
"No weather data found for the given coordinate: " + coordinate)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
IndividualTimeSeries<WeatherValue> timeSeries = | ||
new IndividualTimeSeries<>(timeBasedValues); | ||
coordinateToTimeSeries.put(entry.getKey(), timeSeries); | ||
if (!timeBasedValues.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be great to have equivalent behavior across types of data sources, at least where it's possible
Resolves #554
Added an explicit Exception for handling no received data from WeatherSource to prevent the pipeline moving on without data.