[seismology] Add DepthLookup abstract interface#199
Conversation
|
I am not in favour of the name |
Yeah you are right. Which one you prefer me to go with? |
|
Just thinking, if we already have a class doing a similar thing ... |
051c31b to
744ed10
Compare
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.
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.
Per Jan's review: fetch() and fetchMaxDepth() now throw std::out_of_range when no region/zone contains the given location. Callers that need a fallback value use the utility functions fetchDepth() and fetchMaxDepth() which catch and return the fallback. Removes the fallback ownership from DepthLookupPolygon — callers decide what to do when outside all configured regions.
a886635 to
eff40d7
Compare
|
Addressed Jan's review feedback:
|
|
Do we have the possibility to retrieve more than one depth? I am referring to the situation where there is deep seismicity along a subducting slab, but also shallow seismicity right above it. That separation can be significant and we need to be careful not to trust a potentially wrong depth. |
|
Thank you for the changes. What I still do not like very much are the utility functions. I proposed that initially to provide a fallback rather than incorporating it into the fetch methods. The better design worked out later was that the client ( |
Good point , the current interface only returns a single depth. Are you thinking the interface should return a list of candidate depths for scautoloc to try, or more that scautoloc itself should decide to try both a shallow and a slab-based seed independently? |
Understood, fetch() should always return a value, with the fallback knowledge fully encapsulated in the backend. The client should never need to handle an exception or supply a depth. Will remove the utility functions and have each backend return its own configured fallback when no zone matches. |
|
Why do I have the feeling chatting with Claude AI rather than yourself? ;) |
|
Not really. :) 80% is me, at least.
…On Thu, Jun 4, 2026 at 11:49 PM Jan Becker ***@***.***> wrote:
*gempa-jabe* left a comment (SeisComP/common#199)
<#199 (comment)>
Why do I have the feeling chatting with Claude AI rather than you? ;)
—
Reply to this email directly, view it on GitHub
<#199?email_source=notifications&email_token=ACB5ZSHDHEJRN5WNYALOTCL46F447A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTINRSGI3TIOBWHEZKM4TFMFZW63VGMF2XI2DPOKSWK5TFNZ2KYZTPN52GK4S7MNWGSY3L#issuecomment-4622748692>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACB5ZSBGNTY4IIJAOGUQ73346F447AVCNFSM6AAAAACZGZO4TSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHM2DMMRSG42DQNRZGI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
--
Kind regards
Mustafa
|
Reworked based on Jan's feedback: fetch() and fetchMaxDepth() always return a value, fallback is configured inside the backend. Removed the utility functions that required the client to supply a fallback.
Done, removed the utility functions and the fallback is back where it belongs, inside the backend. fetch() and fetchMaxDepth() always return a value now, the client doesn't know or care about fallbacks. |
|
Sorry for the long silence on this; let me provide a quick status. The last commit (243e39f) moves the fallback fully inside the backend, so fetch() and fetchMaxDepth() always return a value now. The client doesn't catch an exception or supply a default anymore, it doesn't know or care about fallbacks. That's the design we agreed on, so from my side this one is done. On Joachim's multiple-depths point: right now the interface returns back one depth plus a maxDepth bound, and that maxDepth already keeps the client from over-trusting a deep slab. For the shallow-above-slab case I'd rather keep this interface single-depth and let scautoloc try a shallow seed and a slab seed on its own. If you'd sooner have it in the interface, I can add a fetchCandidates() that returns a short list as a follow-up. I just don't want to widen this PR; it's meant to be the minimal foundation. @gempa-jabe When you get a chance, could you have another look? I think it's ready to go. What do you think? :) |
|
I am fine with the code, I left some comments. Furthermore we need a final decision on the multiple depths thing as this will have an impact on the interface which we cannot change within one major release. |
gempa-jabe
left a comment
There was a problem hiding this comment.
See my inline comments.
- Move DepthLookupConstant and DepthLookupPolygon into anonymous
namespace so no concrete-class symbols are exported
- Add fetchCandidates() virtual method with default implementation
returning {fetch(lat,lon)}, resolving Joachim's open question on
multi-depth support without breaking existing backends
|
Addressed in c069340:
|
DepthLookupConstant was returning _value (10 km default) for both
fetch() and fetchMaxDepth(). Any module using fetchMaxDepth() as a
depth-rejection threshold would silently discard all events deeper
than 10 km. Added a separate _maxDepth{1000.0} field configurable
via depths.constant.maxDepth.
Same issue in DepthLookupPolygon: fetchMaxDepth() fell back to
_fallback (10 km) for unmatched locations and for polygons without a
maxDepth attribute. Added _maxDepthFallback{1000.0} configurable via
depths.polygon.maxDepth.
|
Self-review before merge — caught a correctness bug (49e9ca5): fetchMaxDepth returning defaultDepth in both backends
Same issue in |
- DepthLookupPolygon docblock: add depths.polygon.maxDepth key - depthlookup.h class docstring: document maxDepth config keys for both backends; remove reference to dlslab2 plugin (does not exist)
gempa-jabe
left a comment
There was a problem hiding this comment.
If the fetchCandidates prototype is good enough for @jsaul is out of my scope. Please comment on that.
| * contains the given location the backend returns its own | ||
| * configured fallback depth. | ||
| */ | ||
| virtual double fetch(double lat, double lon) const = 0; |
There was a problem hiding this comment.
If a value is always returned, we can add noexcept specifier to the interface to indicate that it always returns a value and never throws. If the methods would throw an exception then a fallback case must be provided at client-side.
There was a problem hiding this comment.
I would expect that fetch() throws if there is no default depth configured for the location and let the client deal with that.
There was a problem hiding this comment.
We have already discussed that starting at that comment. So you do not have to deal with fallback configuration in, e.g. scautoloc or scolv. Ask for a value and get a value anytime you ask.
- Add noexcept to fetch(), fetchMaxDepth(), fetchCandidates() in the interface and all concrete overrides — makes the always-returns contract explicit at the type-system level - DepthLookupPolygon now inherits GeoFeatureSetObserver: registers on init(), stores _names as a member, rebuilds _entries via _buildEntries() on geoFeatureSetUpdated(). GeoFeatureSetObserver destructor handles deregistration automatically. Fixes dangling pointer crash if the global GeoFeatureSet is reloaded (e.g. user reloads map layers in scolv).
|
Addressed in d0d433a: noexcept — added to GeoFeatureSetObserver — |
|
Awesome new world! 😉 |
|
What am I doing now? Which direction? @jsaul ? Are we ok with the current state? |
|
Despite the open question on |
|
To me In principle one might think of ranking the returned depths. If a locator shall try a relocation with each of the returned candidate depths, there may be cases where the results are similar in terms of RMS etc. Such an ambiguity might be resolved by preferring one depth over the other. This might reflect the fact that at the given location far more events occur at the subducting slab interface than in the crust above the slab. Would it perhaps make sense to return |
|
OK I will rename it. The vector idea is also reasonable, particularly regarding the ranking point. But I'd rather keep that for a follow-up once we see how the candidates are actually used in e.g. scautoloc. vector would be straightforward to extend later without breaking existing callers. What do you think @gempa-jabe ?? |
Suggested by Joachim for clarity.
I see your point. But if the |
|
Of course I would like to keep the interface as simple as possible. On the other hand we cannot always anticipate what is needed in the future. At least I cannot. Extending In the end this shall not only be used in scautoloc but wherever default depths are needed and I don't know if |
|
That said for the current problem to be solved, |
|
I will squash all commits and merge them. Smaller changes I will apply myself. Thanks. |
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