refactor: Remove globalToLocal log and return actual error message

[osbornjd]

This is a very simple PR to change this logging message from FATAL level to WARNING. This is leftover from when the globalToLocal function did not return an Acts::Result, so if the transformation failed it was actually fatal. This result can now be passed and caught, thus it isn't necessary to flag it as FATAL.

[codecov]

Codecov Report

Merging #1310 (9ebeb84) into main (0717e5b) will increase coverage by 0.00%.
The diff coverage is 0.00%.

@@           Coverage Diff           @@
##             main    #1310   +/-   ##
=======================================
  Coverage   47.42%   47.43%           
=======================================
  Files         375      375           
  Lines       19788    19787    -1     
  Branches     9287     9285    -2     
=======================================
  Hits         9385     9385           
+ Misses       4021     4020    -1     
  Partials     6382     6382           

Impacted Files	Coverage Δ
Core/src/Propagator/detail/CovarianceEngine.cpp	47.79% <0.00%> (+0.34%)	⬆️

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[paulgessinger]

Should the function not return the error in this case? The way I read the code now, it will just try to continue after the WARNING.

[osbornjd]

Should the function not return the error in this case? The way I read the code now, it will just try to continue after the WARNING.

Ha, good point... this really must be leftover then from when this piece of code just threw a FATAL log and then crashed. I'll change it to not log anything and instead return the error message similar to the track parameters.

What is particularly interesting about your comment is that, in the instances where we see this fatal log message come up, the code does continue on without any sort of issue. So apparently this is handled appropriately somewhere - just not right here (as it probably should be)

[asalzburger]

While we actually do check that globalToLocal could fail and thus handle it with a Result object, we do not do so (currently) for the local to global, because it's more difficult to screw this on up.

  /// Local to global transformation
  /// Generalized local to global transformation for the surface types. Since
  /// some surface types need the global momentum/direction to resolve sign
  /// ambiguity this is also provided
  ///
  /// @param gctx The current geometry context object, e.g. alignment
  /// @param lposition local 2D position in specialized surface frame
  /// @param momentum global 3D momentum representation (optionally ignored)
  ///
  /// @return The global position by value
  virtual Vector3 localToGlobal(const GeometryContext& gctx,
                                const Vector2& lposition,
                                const Vector3& momentum) const = 0;

  /// Global to local transformation
  /// Generalized global to local transformation for the surface types. Since
  /// some surface types need the global momentum/direction to resolve sign
  /// ambiguity this is also provided
  ///
  /// @param gctx The current geometry context object, e.g. alignment
  /// @param position global 3D position - considered to be on surface but not
  /// inside bounds (check is done)
  /// @param momentum global 3D momentum representation (optionally ignored)
  /// @param tolerance optional tolerance within which a point is considered
  /// valid on surface
  ///
  /// @return a Result<Vector2> which can be !ok() if the operation fails
  virtual Result<Vector2> globalToLocal(
      const GeometryContext& gctx, const Vector3& position,
      const Vector3& momentum,
      double tolerance = s_onSurfaceTolerance) const = 0;

[osbornjd]

I agree that the globalToLocal functionality should return a result as Andi said today - the point of this PR is just to have it actually return a result in the event that his check fails. Currently it just issues a FATAL warning and then continues on.

In fact I am sure that this FATAL log is actually caught because there are instances where it is thrown for us in a propagation application I wrote, and the code does not crash (whereas it used to crash completely following this FATAL log).

[paulgessinger]

Sounds good. Can you maybe adjust the PR title and descriptions to reflect the updated nature of the changes? Aside from that lgtm.

[osbornjd]

Sounds good. Can you maybe adjust the PR title and descriptions to reflect the updated nature of the changes? Aside from that lgtm.

Sure, done.