jainankitk commented on code in PR #14516:
URL: https://github.com/apache/lucene/pull/14516#discussion_r2055278522
##########
lucene/core/src/java/org/apache/lucene/util/SloppyMath.java:
##########
@@ -178,6 +178,30 @@ public static double asin(double a) {
}
}
+ // some sloppyish stuff, do we really need this to be done in a sloppy way?
+ // unless it is performance sensitive, we should try to remove.
+ // This is performance sensitive, check SloppySinBenchmark and
RectangleBenchmark
+ private static final double PIO2 = Math.PI / 2D;
+
+ /**
+ * Returns the trigonometric sine of an angle converted as a cos operation.
+ *
+ * <p>Note that this is not quite right... e.g. sin(0) != 0
+ *
+ * <p>Special cases:
+ *
+ * <ul>
+ * <li>If the argument is {@code NaN} or an infinity, then the result is
{@code NaN}.
+ * </ul>
+ *
+ * @param a an angle, in radians.
+ * @return the sine of the argument.
+ * @see Math#sin(double)
+ */
+ public static double sin(double a) {
Review Comment:
> Personally I think an easier solution would be to "demote" it from
GeoUtils to a package-private static method: seems like it is only used by
Rectangle and it doesn't need to be public. This would be easier than "properly
supporting it" IMO.
Thanks @rmuir for the suggestions. While I agree it is easier this way, but
IMO:
* It is better to have related stuff in single place, even if we are using
only once currently
* The benchmarks will be unable to use if it is private, and I feel they do
add some value, in case some else like myself wonders if we really need
sloppySin!?
> Do we really need to promote this from an experimental API in GeoUtils to
a non-experimental public API here?
If we really want to do this, let's keep in line with the existing methods
in the file, and provide accuracy bounds and tests?
Had not realized myself that this promotes to non-experimental API. I have
been bit sloppy with this SloppyMath stuff! :( Thanks for pointing and
patiently reviewing so far.
That being said, I don't see any real harm with `sloppySin` getting promoted
to `SloppyMath`. I have added accuracy bounds and tests. Verified that the test
ran without failure for 1000 iterations
```
> Task :lucene:core:test
:lucene:core:test (SUCCESS): 1000 test(s)
> Task :lucene:core:wipeTaskTemp
The slowest suites (exceeding 1s) during this run:
9.46s TestSloppyMath (:lucene:core)
BUILD SUCCESSFUL in 25s
246 actionable tasks: 175 executed, 71 up-to-date
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
