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+
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:7.4.0 reviewed:2022 target:7.5.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-11-26 22:40 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.
Comment 9 Hossein 2022-06-16 12:58:23 UTC
Re-evaluating the EasyHack in 2022

This issue is still relevant, as there are many places in the code that a Pythagorean addition is done, and std::hypot can be used instead.
Comment 10 Commit Notification 2022-09-03 18:49:14 UTC
Aleksa Savic committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/519dae19abddaedaf0d3d109186eeb4a08471a92

tdf#147906 Use std::hypot for Pythagorean addition

It will be available in 7.5.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 11 Commit Notification 2022-11-18 23:49:19 UTC
OmkarAcharekar committed a patch related to this issue.
It has been pushed to "master":

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

tdf#147906 Use std::hypot for Pythagorean addition

It will be available in 7.5.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 12 Commit Notification 2022-11-24 20:56:27 UTC
Bogdan B committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/1e9ace4c35e4206c24ded230433915850a1f04d1

tdf#147906 used std::hypot for Pythagorean addition

It will be available in 7.5.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 13 Commit Notification 2022-11-26 11:13:39 UTC
Bogdan B committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/6bf5dd8faea420aa0dbc7c429952c9868bcadb9e

tdf#147906 used std::hypot for Pythagorean addition

It will be available in 7.5.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 14 Commit Notification 2022-11-26 22:40:20 UTC
Bogdan B committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/5b18eebc2c95321ce7e6edf10f4df81557382a48

tdf#147906 used std::hypot for Pythagorean addition

It will be available in 7.5.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.