Skip to content
This repository has been archived by the owner on Feb 8, 2024. It is now read-only.

Use precalculated factors #17

Merged
merged 4 commits into from
Apr 9, 2020
Merged

Use precalculated factors #17

merged 4 commits into from
Apr 9, 2020

Conversation

Jozo132
Copy link
Contributor

@Jozo132 Jozo132 commented Apr 9, 2020

Replacing floating point division with floating point multiplication for faster data processing in main loop.

Replaced floating point division with floating point multiplication for execution time reduction.
About 3x faster math operation.
Replaced floating point division with floating point multiplication for execution time reduction.
About 3x faster math operation.
Use precalculated factors (b)
Use precalculated factors (a)
@gabriel-milan
Copy link
Owner

Hi @Jozo132, thanks for the collaboration.

Although I enjoy the idea of this PR, if I do merge this I think I'd have an issue:

  • On 18bd718 you set 1 / 16384.0 as 0.00006103515 and that's accurate, but
  • on 87c8f1c you change it from division to multiplication.

As the values are numerically equal, if you change the operation to the inverse one, results will change.

For that to work, you have either to leave the *_TRANSFORMATION_NUMBER as they are and change the operation to multiplication, or the opposite.

Since the whole idea is to increase data processing speed, the best thing to do is the first one. Do you wish to fix that and open a new PR?

@Jozo132
Copy link
Contributor Author

Jozo132 commented Apr 9, 2020

Hi @Jozo132, thanks for the collaboration.

Although I enjoy the idea of this PR, if I do merge this I think I'd have an issue:

  • On 18bd718 you set 1 / 16384.0 as 0.00006103515 and that's accurate, but
  • on 87c8f1c you change it from division to multiplication.

As the values are numerically equal, if you change the operation to the inverse one, results will change.

For that to work, you have either to leave the *_TRANSFORMATION_NUMBER as they are and change the operation to multiplication, or the opposite.

Since the whole idea is to increase data processing speed, the best thing to do is the first one. Do you wish to fix that and open a new PR?

As you can see, the idea was to replace divisions with multiplication.
So if this abstraction holds true:
y = k / x <==> y = k * (1/x)
that means that a constant z = 1/x simplifies all repeating divisions to multiplications
y = k * z

My 'GIT' skills are terrible and I probably messed up somewhere.
Could you please edit the changes to make it work out?

@Jozo132 Jozo132 closed this Apr 9, 2020
@Jozo132 Jozo132 reopened this Apr 9, 2020
@Jozo132
Copy link
Contributor Author

Jozo132 commented Apr 9, 2020

Oops, sorry ... I thought that was the 'Close' window for the comment input

Anyway, now that I think about what you asked me. No.
It is correct. The division has to be replaced in the cpp file.
The explanation just above should be enough.

@gabriel-milan
Copy link
Owner

Omg my mistake.

When I read your PR's code, I looked on the commentaries right after your code line:

// (1 / 16384) precalculated

and assumed that 1 / 16384 was the previous value set. In that case, my first comment would make sense but yeah, you're right.

I'll merge this PR and publish a new release right after, thanks again and sorry for the misunderstanding.

@gabriel-milan gabriel-milan merged commit b613d8c into gabriel-milan:master Apr 9, 2020
@Jozo132
Copy link
Contributor Author

Jozo132 commented Nov 6, 2022

@gabriel-milan I might have spotted a major issue. I have requested this pull request for each file sperately but we only merged ONE!!
When I just scrolled through your code I noticed the divisions were not replaced with multiplications but the constants were changed!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants