On Tue, 27 Aug 2024 22:44:43 GMT, Joe Darcy <[email protected]> wrote:
>>> This PR doesn't include any additional tests. It is often appropriate to
>>> add more regression testing when introducing a new implementation of a
>>> method.
>>
>> Thank You Joe for the suggestion. Will add more tests. (This PR passes the
>> tier-1 tanh tests in the HyperbolicTests.Java)
>
>> > This PR doesn't include any additional tests. It is often appropriate to
>> > add more regression testing when introducing a new implementation of a
>> > method.
>>
>> Thank You Joe for the suggestion. Will add more tests. (This PR passes the
>> tier-1 tanh tests in the HyperbolicTests.Java)
>
> Yes @vamsi-parasa ; running that test is a good backstop and it is written to
> be applicable to any implementation of {sinh, cosh, tanh} that meet the
> general quality-of-implementation criteria for java.lang.Math. To be
> explicit, the WorstCaseTests.java file, and for good measure all the
> java.lang.Math tests, should also be run too for a change like this.
>
> For a hypothetical example, if an intrinsic used different polynomials for
> different ranges of the input, it would be a reasonable regression tests _for
> that implementation_ to probe around the boundary of the transition between
> the polynomials to make sure the monotonicity requirements were being met.
>
> That kind of check could be written to be generally applicable and be
> suitable for a regression tests in java/lang/Math or could be suitable for a
> regression test in the HotSpot area. HTH
Hi Joe(@jddarcy) and Andrew (@theRealAph) ,
Please see the updates below:
> This PR doesn't include any additional tests. It is often appropriate to add
> more regression testing when introducing a new implementation of a method.
>
Added 1500 regression tests in HyperbolicTests.java which compare the accuracy
of the Math.tanh intrinsic by using StrictMath.tanh (which calls
FdLibm.Tanh.compute) as a reference. The tests are passing within 2.5 ulps of
the expected result. The tests are fairly exhaustive and also cover the
boundary transitions.
> Yes @vamsi-parasa ; running that test is a good backstop and it is written to
> be applicable to any implementation of {sinh, cosh, tanh} that meet the
> general quality-of-implementation criteria for java.lang.Math. To be
> explicit, the WorstCaseTests.java file, and for good measure all the
> java.lang.Math tests, should also be run too for a change like this.
>
Ran the WorstCaseTests.java and all the tests in java.lang.Math and they're
passing on my local machine.
> For a hypothetical example, if an intrinsic used different polynomials for
> different ranges of the input, it would be a reasonable regression tests _for
> that implementation_ to probe around the boundary of the transition between
> the polynomials to make sure the monotonicity requirements were being met.
>
Added new tests in HyperbolicTests.java which probe around the various
boundaries of transition. 1500 testcases and they passed within 2.5ulps of the
reference StrictMath.tanh
> That kind of check could be written to be generally applicable and be
> suitable for a regression tests in java/lang/Math or could be suitable for a
> regression test in the HotSpot area. HTH
Please let me know if anything more needs to be added.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/20657#issuecomment-2322295827
