SeisComP / common

SeisComP framework C++ libraries, Python wrappers and messaging
Other
8 stars 21 forks source link

Should PickerView and AmplitudeView consider the hypocenter distance? #80

Closed luca-s closed 2 months ago

luca-s commented 1 year ago

I am working on two projects where we have several boreholes with multiple sensors per borehole. In this scenario, when I use the PickerView or the AmplitudeView the "sorting by distance" feature doesn't work well because the GUIs consider only the epicenter distance instead of the hypocenter distance and the events are so close to the sensors that the difference in their elevation matters.

I was considering providing a PR to fix this, but after reviewing the code of PickerView and AmplitudeView I have the feeling that they were never meant to be used for such local processing where the hypocenter vs epicenter distance is relevant.

Do you think it would make sense to fix this use case?

gempa-jabe commented 1 year ago

I totally forgot about this one. Is it still valid for you?

gempa-jabe commented 1 year ago

If so how do you think that both components were never meant to support your request? Any hints in the code that are an show stopper?

luca-s commented 1 year ago

Hi @gempa-jabe. I still believe this could be an interesting enhancement for SeisComP, as long as you are interested in the usage of SeisComP at low scale.

However if we want to tackle the problem in a consistent way there are a couple of places in the code where the epicenter vs hypocenter distance plays a role that we need to fix:

Overall, I believe SeisComP would perfectly work at low scale since I am using it for that purpose, but the code might need some change here and there to remove the current assumptions that we are working at regional scale and station elevations do not matter.

gempa-jabe commented 3 months ago

As this feature is gaining more momentum currently, I would suggest the following approach to this:

Would that help your use case already?

luca-s commented 3 months ago

Having a global configuration option scheme.distanceHypocentral seems a good approach to me. This would definitely work fine with me and that would solve my issues that I currently tackle by locally patching the code

gempa-jabe commented 3 months ago

the distance information is computed by the locator and all locators (except for stdloc) compute the epicenter distance.

That being said, please see my comment above. Arrival.distance is defined as epicentral distance. You can use the hypocentral distance internally but not to populate the distance attribute.

luca-s commented 3 months ago

Yes, thanks. I have already fixed that .

gempa-jabe commented 3 months ago

:+1:

luca-s commented 3 months ago

Also my comment on the magnitude was off ("one might argue the change is towards more correct values"). I now understand that the magnitude computation needs epicentral distance and depending on the bindings configuration the mag processor decides to use that epicentral distance or it computes the hypocentral one.

gempa-jabe commented 3 months ago

@luca-s I created a first version of the feature that "should" work in scolv location tab, the picker and the amplitudes. Are you interested in testing it? Then I can push it to a branch. Otherwise I do it for myself and push it later to main.

gempa-jabe commented 3 months ago

Also my comment on the magnitude was off

The amplitude picker passes the distance to the magnitude processor and that would be different, you were right. We have to push epicentral distance explicitly.

luca-s commented 3 months ago

Thanks @gempa-jabe but I cannot test it right now, because I am stuck with seiscomp v5. Eventually I will move to seiscomp 6 and I will report of any issue I find. That is not happening soon though

gempa-jabe commented 2 months ago

The feature is now in main and if you ever want to test it, it is ready for you. I am closing the issue and you can re-open it if necessary later.

luca-s commented 2 months ago

@gempa-jabe are you planning to release this feature on v6 branch?

gempa-jabe commented 2 months ago

This is not possible due to ABI changes.