SeisComP / common

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

[core] allow null arguments to Seiscomp::Math::Geo::delazi #106

Closed luca-s closed 1 year ago

luca-s commented 1 year ago

This is a small improvements for the next API revision. This PR allows to pass null arguments to Seiscomp::Math::Geo::delazi: either one of dist, azi or baz can be omitted.

That would make the code nicer in many places and it will also save some computations: the trigonometric functions can be expensive and sometimes are called millions of times.

jsaul commented 1 year ago

Computationally, there is indeed a significant saving of probably around 20-30 percent if none of the azimuths is needed. If any of the azimuths is needed, practically no saving is possible even if the distance is not needed.

I would support the proposed changes, even though I find the use of null pointers as switches rather awkward and not really helpful to improve code readability. Therefore I would propose to also provide a delta() function as a convenience wrapper around delazi(). Within the delta() function delazi() is called with null pointers for the two azimuths. Computationally delazi() is so expensive anyway that the little wrapper code is irrelevant.

luca-s commented 1 year ago

Instead of providing a 'delta()' function why don't we provide default values ('nullptr') to 'delazi()'? That would be even nicer when only az or baz is needed

I find the use of null pointers as switches rather awkward and not really helpful to improve code readability

The readability improves on all places where the function is called without the intent of fetching all 3 of dist, az and baz. Currently when you read the code it is not obvious to understand what the code needs: dist az or baz?

gempa-jabe commented 1 year ago

I would like to have a function which returns the distance as return value of the function. The return value of _delazi is always 0 and does not make any sense. delazi with default values and delta as wrapper which returns the distance right away.

luca-s commented 1 year ago

Updated the PR

luca-s commented 1 year ago

Since I am on it and testing it, I will also avoid the computation of baz if that is not requested (and vice versa for azi).

luca-s commented 1 year ago

I cannot do that, I always need to compute both angles if one is requested because of this check:

if ((isNaN(azi1) || isNaN(azi2)) && fabs(lon2-lon1)<0.000001)

So the PR is ready if you are happy too