Improvements to Complex trig and hyperbolic trig functions #15835
Conversation
stephentoub
changed the title
| return (Sin(value) / Cos(value)); | ||
| // tan z = sin z / cos z, but to avoid unnecessary repeated trig computations, use | ||
| // tan z = \frac{\sin(2x) + i \sinh(2y)}{\cos(2x) + \cosh(2y)} | ||
| // (see from Abromowitz & Stegun 4.3.57 or derive by hand), and compute trig functions here. |
| @@ -271,13 +278,40 @@ public static Complex Acos(Complex value) | |||
|
|
|||
| public static Complex Tan(Complex value) | |||
| { | |||
| return (Sin(value) / Cos(value)); | |||
| // tan z = sin z / cos z, but to avoid unnecessary repeated trig computations, use | |||
| // tan z = \frac{\sin(2x) + i \sinh(2y)}{\cos(2x) + \cosh(2y)} | |||
There was a problem hiding this comment.
I don't find this notation particularly readable in a code comment. Could you re-write it? I think this would be more readable, for example:
tan(z) = (sin(2x) + i * sinh(2y)) / (cos(2x) + cosh(2y))
// or even
tan(z) = (sin(2x) + i * sinh(2y))
/ (cos(2x) + cosh(2y))There was a problem hiding this comment.
This is just by TeX background showing though. Agreed, for formulas this simple, it's unnecessary. Will change in PR update.
|
Going forward with changes like this, I'd actually like to do something slightly different with the tests, in order to save us some work at a later time:
When/if we port this change back into .NET Framework, we will most likely need to put it behind a "quirk", which means that both the new and old behavior could be present, depending on how a project is configured. Having both old and new tests around will allow us to easily test both cases of the quirk. Make sense? |
|
@mellinoe: I'm happy to structure behavior and test changes as you request, but I'd like to understand the implications better. The previous guidance was just to mark tests that changed previous behavior as skip-on-netfx. When the new behavior was ported, that attribute would be removed, and the new tests would run everywhere. The new guidance is to leave old tests in place but mark them as specific-to-netfx. Is there also a standard procedure to delete such tests after porting? Because otherwise those tests will suddenly break after the port. I'd also strongly urge against establishing the old behavior as an optional quirk. You might think from looking at the tests that this just involved behavior at infinity and maybe maxvalue, but in fact the old code gives wrong results even for very everyday values. For example, the old code correctly gives Tan(710.0 i) = 1.0 i, but then incorrectly gives Tan(711.0 i) = NaN, whereas the right answer is 1.0 i. This isn't a quirk, it's a bug. I assume you don't create an optional quirk for every bug fix. A quirk setting would be appropriate if the old behavior was considered, documented, and sensible, but just needed to be changed to reflect a standard, but this not that case. The fact that someone wrote test cases that assert the old behavior might make it look like such a case, but in fact the old test assertions were just assertions of whatever the old code produced, not something that came from any considered, documented, and sensible requirement external to the old code. |
We don't do it for EVERY fix, but we have an extremely high bar for any observable changes like this on the .NET Framework. @AlexGhiondea could better explain the kinds of changes we find acceptable to do without a quirk.
We will add quirks if we believe that the change could possibly break any existing code out in the wild. The .NET Framework is updated in-place, and we don't consider it acceptable to make a change which could break ANY application without that developer's intervention, even if that application is relying on "broken" or "buggy" behavior in order to work correctly. It's a huge burden, yes, but it's the price we pay for having the .NET Framework bundled and updated with Windows. |
|
@mellinoe: Okay. I'm trying to update the PR to conform to your guidance, but I think you must mean a slightly different attribution than you wrote. If I use the PlatformSpecific attribute I can say Windows or AnyUnix, but not NetFramework. If I say Windows, then the legacy tests run and, of course, return failure. If I use SkipOnTargetFramework I can specify NetFramework, but I don't want to skip the legacy tests on netfx, I want to run the legacy tests on netfx. Can you either: (i) give me the correct attribution for legacy tests or (ii) say "not for this check-in"? :-) @AlexGhiondea: I would like to understand the specific requirements for a quirks settings better, since any change, even e.g. making a function execute faster without changing its outputs, could be "breaking" in this sense. |
|
Sorry, the attributes should be more along these lines: [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)]
// New behavior
[SkipOnTargetFramework(~TargetFrameworkMonikers.NetFramework)]
// Old behavior
`` |
|
@dcwuser here is how we think about breaking changes. When porting code to Full .NET, we typically introduce a quirk for every change that has the potential to break existing code. Since Full .NET is an in-place update, code that used to rely on the old behavior needs to continue to run with the newer version of .NET. We typically put the quirk in place when we are porting the change over to Full .NET. |
|
@dotnet-bot test code coverage please |
| double sinh = (p - q) * 0.5; | ||
| double cosh = (p + q) * 0.5; | ||
| return new Complex(Math.Sin(value._real) * cosh, Math.Cos(value._real) * sinh); | ||
| // There is a small region where sinh and/or cosh (correctly) overflow, while sin and/or cos are small enough |
There was a problem hiding this comment.
Again, I think this sort of text should be left as a comment to the PR rather than preserved in the source code. Can we just shorten it to something simple like
There is a known limitation with this algorithm; it returns (Infinity) for cases x, y, z.
There was a problem hiding this comment.
Addressed all these requests in the latest PR update. Just on this one I left a little more info in the code comment than I think you might have preferred, because I really want someone reading the comment to know the nature, not just the existence, of the limitation. I did include a specific example as you requested.
| // tan z = (sin(2x) + i sinh(2y)) / (cos(2x) + cosh(2y)) | ||
| // (see Abramowitz & Stegun 4.3.57 or derive by hand), and compute trig functions here. | ||
|
|
||
| // Even this can only be used for |y| not-too-big, beacuse sinh and cosh (correctly) overflow |
There was a problem hiding this comment.
The wording is a little awkward. Consider:
The approach does not work for moderately-large values of |y|, because sinh and cosh will overflow. In that case, divide through by ...
| yield return new object[] { double.NaN, double.PositiveInfinity, double.NaN, double.NaN }; | ||
| yield return new object[] { double.NaN, double.NaN, double.NaN, double.NaN }; | ||
|
|
||
| // Simple regression test; previous code returned wrong result for this argument |
There was a problem hiding this comment.
You can leave off this comment; I think it just confuses things to talk about what "previous code" did.
|
|
||
| public static IEnumerable<object[]> Tan_Legacy_TestData() | ||
| { | ||
| // These tests assert previous behavior which is not mathematically correct. |
There was a problem hiding this comment.
"These cases validate legacy .NET Framework behavior"
|
@dotnet-bot test code coverage please |
|
Thanks for making the suggested changes, @dcwuser , and thanks again for the great contribution. I know the test changes seem a little tedious, and perhaps backwards in a way, but I do believe it will help save us quite a bit of headache in the future. And, at the very least, it keeps our test coverage on .NET Framework which will still have the old behavior. |
Improvements to Complex trig and hyperbolic trig functions Commit migrated from dotnet/corefx@8c0ea60
This is a series of relatively minor improvements to the complex trig functions and complex hyperbolic trig functions.
Sin and Cos perf is improved by only making one call to Math.Exp to compute the required real hyperbolic trig functions. (Hopefully the JIT compiler is smart enough to make a single FPU call for the required real sin and cos, but that's not something I can force. It would be nice if there were a Math.SinAndCos method that explicitly forced this.)
Tan perf is similiarly improved, and, more importantly, modified to correctly handle the large region (all |z.Im| larger than about 710) for which Sin and Cos overflow but Tan doesn't.
Sinh, Cosh, and Tanh are modified to call Sin, Cos, and Tan, so that all current and future improvements to the complex trig functions automatically feed into the complex hyperbolic trig functions.
Several test cases for Tan and Tanh involving large arguments needed to be changed. Some of the previous test assertions were simply assertions of the previous bad behavior, e.g. demanding NaN for Tanh(big real) instead of 1. These have been changed to assert the new, mathematically correct behavior. Some of the previous test assertions are inappropriate given the bad behavior of the .NET real trig functions, e.g. demanding Tan(x + 0.0 i) = Math.Sin(x) / Math.Cos(x) for big x, even though Math.Sin(x) and Math.Cos(x) return incorrect values for big x. I have simply removed these tests. I have added attributes to skip all the changed tests for NetFx; there are still many shared test assertions for not-big arguments that are satisfied by both new and old code.
(The fix for Asin and Acos come next. I wanted to get these simple fixes for the basic trig functions in before submitting a complicated one for the inverse trig functions.)