[seismology] Add DepthLookup abstract interface#199
Conversation
| @@ -0,0 +1,161 @@ | |||
| /*************************************************************************** | |||
| * Copyright (C) GFZ Potsdam * | |||
There was a problem hiding this comment.
This is the wrong copyright header for code in common. Please see ttt.h for example.
| const std::string &key) { | ||
| const auto &attrs = f->attributes(); | ||
| auto it = attrs.find(key); | ||
| if ( it == attrs.end() || it->second.empty() ) |
There was a problem hiding this comment.
Please use brackets also for blocks containing just one statement.
| namespace Seismology { | ||
|
|
||
|
|
||
| DEFINE_SMARTPOINTER(DefaultDepthSetter); |
There was a problem hiding this comment.
We need to resolve the class name discussion first.
|
I am not in favour of the name |
| * @param fallback Returned when no region/slab matches. | ||
| */ | ||
| virtual double getDefaultDepth(double lat, double lon, | ||
| double fallback) const = 0; |
There was a problem hiding this comment.
We usually return a success flag or throw an exception if a value is present. We do not follow the "fallback" approach. The latter can be implemented by a utility function.
class DepthLookup {
public:
double fetch(double lat, double lon) {
throw runtime_error("Some error");
}
}
double getDepth(DepthLookup *provider, double lat, double lon, double fallback) {
try {
return provider->fetch(lat, lon);
}
catch ( ... ) {
return fallback;
}
}But we can discuss whether such behaviour is necessary or not. A depth-provider could always return something based on its configuration and implement the fallback itself. If you use the "constant" provider, it does not make sense to always return the fallback as this does not come from the provider itself but from the client. So it is actually not a constant-provider but a fallback- or none-provider which could be easily accomplished by using no provider and always use the client-side configured fallback.
The question is, how much "knowledge" about depths you would like to add to the client (e.g. scautoloc) itself and/or to the provider. If we have such provider interface, I prefer to keep the entire depth knowledge encapsulated there and the client itself does not have any configuration regarding default depths and so on. It just requests a depth from the provider and done. It is guaranteed that it will return a value in any case. That will make the configuration consistent across different clients and removes the additional client-specific fallback configuration.
I could envision the following configuration:
defaultDepth = constant://10
or
defaultDepth = polygon:///path/to/poly?fallback=10
If URLs are not the way to go, then, e.g.:
defaultDepth = polygon
# Configure polygon backend
depths.polygon.file = /path/to/poly
depths.polygon.fallback = 10
# Configure constant backend
depths.constant.value = 10
There was a problem hiding this comment.
Thanks for the detailed feedback. A few follow-up questions before I rework the interface:
Happy to go with DepthLookup.
API / fallback ownership: I understand the preference, the provider owns all depth knowledge, and fetch(lat, lon) always returns a value with no fallback parameter on the client side.
Two questions on that:
Config namespace: would you prefer a shared depths..key prefix (as in your example) or is it fine for each backend has their own namespace (e.g. ddslab2.directory, ddslab2.fallback)?
maxDepth: scautoloc uses a maximum depth cut-off in its origin acceptance check . Should the interface carry a second fetchMaxDepth(lat, lon) method, or is that out of scope for this interface and better handled separately?
No-provider case: if autoloc.depthLookup is unset, should the factory return nullptr and scautoloc fall back to its own single constant, or should there always be a configured provider (e.g. constant as the required default)?
There was a problem hiding this comment.
Config namespace: would you prefer a shared depths..key prefix (as in your example) or is it fine for each backend has their own namespace (e.g. ddslab2.directory, ddslab2.fallback)?
Each backend should have definitely its own namespace. In addition to, e.g. the travel time table configuration, a global depths namespace would make sense. Also w.r.t. to the configuration XML to avoid name clashes with existing parameter names.
maxDepth: scautoloc uses a maximum depth cut-off in its origin acceptance check . Should the interface carry a second fetchMaxDepth(lat, lon) method, or is that out of scope for this interface and better handled separately?
Here I don't have the experience to answer that. The question is for me, how is that maximum depth handled? Is it more that a client is interested in a valid depths range in a particular region?
No-provider case: if autoloc.depthLookup is unset, should the factory return nullptr and scautoloc fall back to its own single constant, or should there always be a configured provider (e.g. constant as the required default)?
Exactly my point. I would remove the default depth configuration from scautoloc entirely and rely on a configured backend. No backend, no running scautoloc instance.
Yeah you are right. Which one you prefer me to go with? |
|
Just thinking, if we already have a class doing a similar thing ... |
Adds an extensible depth-lookup interface used by scautoloc and other processing modules to select region- or slab-specific default and maximum depths instead of a single global value. Two built-in implementations: "Constant" (passthrough) and "Polygon" (named GeoFeatureSet regions with defaultDepth/maxDepth attributes). Third-party backends register via REGISTER_DEPTH_LOOKUP in a plugin.
051c31b to
744ed10
Compare
Rename getDefaultDepth/getMaxDepth → fetch/fetchMaxDepth; remove caller-supplied fallback parameter. Each backend now owns its full depth knowledge including fallback via its own config namespace: depths.constant.value, depths.polygon.regions, depths.polygon.fallback.
Summary
Introduces an abstract
DepthLookupinterface for region- and slab-baseddepth lookup in SeisComP processing modules (primarily scautoloc).
Interface (
libs/seiscomp/seismology/depthlookup.h):Each backend owns all depth knowledge including fallback; no fallback
parameter on the caller side.
Two built-in backends (
depthlookup.cpp):"Constant"— returnsdepths.constant.valuefor any location"Polygon"— queries GeoFeatureSet polygons (each must carry adefaultDepthattribute); fallback fromdepths.polygon.fallback;regions listed in
depths.polygon.regionsA third
"Slab2"backend ships in SeisComP/main as thedlslab2plugin (see SeisComP/main#121).
Test plan
seiscomp_core— clean compile, no errorsDepthLookupConstantreturnsdepths.constant.valueDepthLookupPolygonreturns polygon depth inside region, fallback outside