Summary: | Calc - loss of imaginary part in IMPOWER() result | ||
---|---|---|---|
Product: | LibreOffice | Reporter: | Dominique PAUL <dominiqu.jc.paul> |
Component: | Calc | Assignee: | Daniel Thomas <daniel> |
Status: | NEW --- | ||
Severity: | normal | CC: | buzea.bogdan, daniel, erack, mentoring |
Priority: | medium | Keywords: | difficultyMedium, easyHack, skillCpp |
Version: | Inherited From OOo | ||
Hardware: | x86-64 (AMD64) | ||
OS: | Linux (All) | ||
Whiteboard: | |||
Crash report or crash signature: | Regression By: | ||
Bug Depends on: | |||
Bug Blocks: | 108253 | ||
Attachments: | calc file with the reproductible bug |
Description
Dominique PAUL
2023-10-16 11:07:51 UTC
Created attachment 190236 [details]
calc file with the reproductible bug
It's looking similar in 7.0, 5.2 and 3.5. I won't change the status as I'm not an expert. I think this may call for a rewrite of the sca::analysis::Complex::* functions using the meanwhile available C++ std::complex type and its functions. In this case, in Complex::Power() https://opengrok.libreoffice.org/xref/core/scaddins/source/analysis/analysishelper.cxx?r=31486f92#1696 the Complex::Abs() (implemented by std::hypot(r,i)) with r=0.99999993365635909 and i=-5.6971371894503506e-09 returns p=0.99999993365635909 and phi=acos(r/p) then correctly returns phi=0 that with i=sin(phi)*p obviously leads to no imaginary part. While the theory behind the implementation is correct, numeric cancellation of smallest numbers seems to come into effect here. Let's hope the ::std implementation does better, here std::complex<double> std::pow( const std::complex<double>& x, const double& y ) To be investigated and verified. The results at least shouldn't be worse.. Then similar in Complex::Abs() use double std::abs( const complex<double>& z ) and so on. Flagging as EasyHack. Additionally we likely want to use rtl::math::stringToDouble() for better conversion of the real and imaginary parts, instead of the self-baked implementation in Complex::ParseString() and ParseDouble(). I would like to have a go at this. The more I think about this, the more I think that we should just make Complex be a wrapper around std::complex as opposed to storing it as r and i. Does that make sense? I guess the alternative would be that each method ends up converting to it and then converting back, e.g. in power: std::complex<double> base(r, i); std::complex<double> result = std::pow(base, fPower); r = result.real(); i = result.imag(); I guess in theory we could abstract that into a separate method but storing complex directly seems cleaner I think The more I think about this, the more I think that we should just make Complex be a wrapper around std::complex as opposed to storing it as r and i. Does that make sense? I guess the alternative would be that each method ends up converting to it and then converting back, e.g. in power: std::complex<double> base(r, i); std::complex<double> result = std::pow(base, fPower); r = result.real(); i = result.imag(); I guess in theory we could abstract that into a separate method but storing complex directly seems cleaner I think Draft PR:https://gerrit.libreoffice.org/c/core/+/160394/1/scaddins/source/analysis/analysishelper.cxx I need to do more automated + manual testing and possibly take a look at stringToDouble I think this is as ready as it'll ever be. I will admit I'm slightly out of m depth with the isValidArcArg() calls, so have basically left them as something similar to what was already there - it's hard to know what cases to test for, especially on x86 and I don't know if the issues still apply, and whether they apply to std::complex Frustratingly, I ccan't find an easy way to get the .fods files tto diff nicely. however, the test changes are: * add a test in impower.fods to replicate the issue * update a test in imsech.fods - the output was previously 1.18503917609399 but shouold be 1.185039176094 - that was a subtle ounding error that seems to have been accidentally fixed (have tested in various places with very long number types to confirm) I have not taken a look at stringToDouble() but may be happy to do so in a separate PR (In reply to Daniel Thomas from comment #9) > I think this is as ready as it'll ever be. I will admit I'm slightly out of > m depth with the isValidArcArg() calls, so have basically left them as > something similar to what was already there - it's hard to know what cases > to test for, especially on x86 and I don't know if the issues still apply, > and whether they apply to std::complex > > Frustratingly, I ccan't find an easy way to get the .fods files tto diff > nicely. however, the test changes are: > * add a test in impower.fods to replicate the issue > * update a test in imsech.fods - the output was previously 1.18503917609399 > but shouold be 1.185039176094 - that was a subtle ounding error that seems > to have been accidentally fixed (have tested in various places with very > long number types to confirm) > > I have not taken a look at stringToDouble() but may be happy to do so in a > separate PR That test failure for the patchset 9 build seems to be due to your patch. Is it something you can address? |