Description: Sometime, the result of COMPLEXE.PUISSANCE(xx;2) has no imaginare part, when it should. Steps to Reproduce: 1.open my file (if you have no file, contact me) 2.see the D219 cell and below 3.you can compare result of D column and H column: they should be very near, but the result is more precise in H. Actual Results: no imaginare part Expected Results: an imaginare part near -1,18814490702782e-08j Reproducible: Always User Profile Reset: No Additional Info: Version: 7.3.7.2 / LibreOffice Community Build ID: 30(Build:2) CPU threads: 4; OS: Linux 5.15; UI render: default; VCL: gtk3 Locale: fr-FR (fr_FR.UTF-8); UI: fr-FR Ubuntu package version: 1:7.3.7-0ubuntu0.22.04.3 Calc: threaded
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
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?