Bug 147906 - Use std::hypot for Pythagorean addition
Summary: Use std::hypot for Pythagorean addition
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
7.4.0.0 alpha0+ Master
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:7.4.0
Keywords: difficultyBeginner, easyHack, skillCpp, topicCleanup
Depends on:
Blocks: Dev-related
  Show dependency treegraph
 
Reported: 2022-03-10 14:42 UTC by Hossein
Modified: 2022-03-31 11:52 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:
Regression By:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Hossein 2022-03-10 14:42:34 UTC
There are many places in the code that calculating the Pythagorean addition is needed, which is defined as:

    a ⊕ b = sqrt(a^2+b^2)

This is hypotenuse, or the length of the longest side of a right-angled triangle, that we know the other 2 sides, a and b.

https://en.wikipedia.org/wiki/Pythagorean_addition

Theoretically, this calculation could be done using sqrt(a * a + b * b), but there are problems with this approach. For large a or b, there is a possibility of overflow, although the result itself is not that big to cause overflow.

To overcome this problem, there are implementations of hypotenuse that do not use power of 2, and use other methods to calculate the result. One of them which is usable in C++ is the std::hypot. To calculate a ⊕ b, you can easily use std::hypot(a,b).

https://en.cppreference.com/w/cpp/numeric/math/hypot

As an example, you can see this commit from Mike:

https://git.libreoffice.org/core/+/4cbaaf21fe1c22b36dd72798cecfa59e73d0f8c3
How to find instances

Among the result of this output, you can find some instances:

    git grep sqrt

but then you should look for things like 'statement * statement' in the results.
Comment 1 Commit Notification 2022-03-18 17:46:53 UTC
Gautham Krishnan committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/67fcd8f268fd04ca2012470af2257b394b77b8fc

tdf#147906 used std::hypot for Pythagorean addition

It will be available in 7.4.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 2 Commit Notification 2022-03-23 19:52:49 UTC
VaibhavMalik4187 committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/d0055ac076c61d24c505c2ffa4f99e36c97b1266

tdf#147906 Use std::hypot for Pythagorean addition

It will be available in 7.4.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 3 Commit Notification 2022-03-28 09:44:30 UTC
offtkp committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/ab9896bfda4d2ef16f3cbb373edced33f9021492

tdf#147906 change all sqrt(a * a + b * b) occurences to std::hypot(a, b)

It will be available in 7.4.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 4 Bartosz 2022-03-28 13:54:25 UTC
It seems that std::hypot is much slower that square root:
https://stackoverflow.com/questions/32435796/when-to-use-stdhypotx-y-over-stdsqrtxx-yy

I am wondering if this change is worth implementing (possible performance drop).
Comment 5 Commit Notification 2022-03-29 07:18:41 UTC
Bartosz Kosiorek committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/a2d49b680cd026c3d64683f6ee3ba97e9834a7f7

tdf#147906 change sqrt(a * a + b * b) occurences to std::hypot(a, b)

It will be available in 7.4.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 6 Libreoffice user SSO 2022-03-29 11:02:55 UTC
At core/filter/source/svg/svgwriter.cxx:304 "const double    fRadius = sqrt( static_cast< double >( rObjRect.GetWidth() ) * rObjRect.GetWidth() + rObjRect.GetHeight()*rObjRect.GetHeight() ) * 0.5;" 

Is it a candidate for changing it to: 
"const double fRadius = std::hypot(rObjRect.GetWidth(), rObjRect.GetHeight()) * 0.5"

If yes, then why it's not letting me make a commit containing this lines of code ?

If no, Some explanations will be great :)!!
Comment 7 Hossein 2022-03-29 11:18:03 UTC
(In reply to Libreoffice user SSO from comment #6)
> At core/filter/source/svg/svgwriter.cxx:304 "const double    fRadius = sqrt(
> static_cast< double >( rObjRect.GetWidth() ) * rObjRect.GetWidth() +
> rObjRect.GetHeight()*rObjRect.GetHeight() ) * 0.5;" 
> 
> Is it a candidate for changing it to: 
> "const double fRadius = std::hypot(rObjRect.GetWidth(),
> rObjRect.GetHeight()) * 0.5"
> 
> If yes, then why it's not letting me make a commit containing this lines of
> code ?
The change itself is OK, if you add ; in the end. Let's discuss this in the dev IRC.
Comment 8 Commit Notification 2022-03-31 11:52:17 UTC
pragat-pandya committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/a31aadb1ef7dfdd4ae76a15707ab51a82c95d868

tdf#147906 Use std::hypot for Pythagorean addition

It will be available in 7.4.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.