Added CORDIC algorithm #7493

zivdar001matin · 2022-10-21T16:19:01Z

Describe your change:

I have added this algorithm so that it can help others to use it and get to know how simply CORDIC works.
Fixes: #7490

Add an algorithm?
Fix a bug or typo in an existing algorithm?
Documentation change?

Checklist:

I have read CONTRIBUTING.md.
This pull request is all my own work -- I have not plagiarized.
I know that pull requests will not be merged if they fail the automated tests.
This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
All new Python files are placed inside an existing directory.
All filenames are in all lowercase characters with no spaces or dashes.
All functions and variable names follow Python naming conventions.
All function parameters and return values are annotated with Python type hints.
All functions have doctests that pass the automated testing.
All new algorithms have a URL in its comments that points to Wikipedia or other similar explanation.
If this pull request resolves one or more open issues then the commit message contains Fixes: #{$ISSUE_NO}.

algorithms-keeper

Click here to look at the relevant links ⬇️

🔗 Relevant Links

Repository:

Contributing guidelines

Project Euler solution guidelines

Python:

Formatted string literals (f-strings)

Type hints

doctest

unittest

pytest

Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.

algorithms-keeper commands and options

algorithms-keeper actions can be triggered by commenting on this PR:

@algorithms-keeper review to trigger the checks for only added pull request files

@algorithms-keeper review-all to trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.

NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.

algorithms-keeper · 2022-10-21T16:19:11Z

other/cordic.py

+        )
+
+    @staticmethod
+    def arctan_div_cordic(x: int, y: int) -> float:


Please provide descriptive name for the parameter: x

Please provide descriptive name for the parameter: y

algorithms-keeper · 2022-10-21T16:19:11Z

other/cordic.py

+        return Cordic.fixed_to_float(Cordic.cordic(x, y, 0, CordicMode.VECTOR)["z"])
+
+    @staticmethod
+    def arctan_cordic(y: int) -> float:


Please provide descriptive name for the parameter: y

algorithms-keeper · 2022-10-21T16:19:11Z

other/cordic.py

+        return Cordic.fixed_to_float(Cordic.cordic(1, y, 0, CordicMode.VECTOR)["z"])
+
+    @staticmethod
+    def rot_decision(mode: CordicMode, value: int) -> bool:


As there is no test file in this pull request nor any test function or class in the file other/cordic.py, please provide doctest for the function rot_decision

algorithms-keeper · 2022-10-21T16:19:12Z

other/cordic.py

+            return not flag
+
+    @staticmethod
+    def cordic(x: int, y: int, z: int, mode: CordicMode) -> dict:


As there is no test file in this pull request nor any test function or class in the file other/cordic.py, please provide doctest for the function cordic

Please provide descriptive name for the parameter: x

Please provide descriptive name for the parameter: y

Please provide descriptive name for the parameter: z

algorithms-keeper · 2022-10-21T16:19:12Z

other/cordic.py

+        return result
+
+    @staticmethod
+    def fixed_to_float(fixed: int) -> float:


As there is no test file in this pull request nor any test function or class in the file other/cordic.py, please provide doctest for the function fixed_to_float

algorithms-keeper · 2022-10-21T16:19:12Z

other/cordic.py

+        return fixed / SHIFT_BASE
+
+    @staticmethod
+    def float_to_fixed(flt: float) -> int:


As there is no test file in this pull request nor any test function or class in the file other/cordic.py, please provide doctest for the function float_to_fixed

CaedenPH

Small changes, but otherwise looks awesome

other/cordic.py

Co-authored-by: Caeden Perelli-Harris <[email protected]>

algorithms-keeper

Click here to look at the relevant links ⬇️

🔗 Relevant Links

Repository:

Contributing guidelines

Project Euler solution guidelines

Python:

Formatted string literals (f-strings)

Type hints

doctest

unittest

pytest

Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.

algorithms-keeper commands and options

algorithms-keeper actions can be triggered by commenting on this PR:

@algorithms-keeper review to trigger the checks for only added pull request files

@algorithms-keeper review-all to trigger the checks for all the pull request files, including the modified files. As we cannot post review comments on lines not part of the diff, this command will post all the messages in one comment.

NOTE: Commands are in beta and so this feature is restricted only to a member or owner of the organization.

algorithms-keeper · 2022-10-21T16:32:46Z

other/cordic.py

+        )
+
+    @staticmethod
+    def arctan_div_cordic(x: int, y: int) -> float:


Please provide descriptive name for the parameter: x

Please provide descriptive name for the parameter: y

algorithms-keeper · 2022-10-21T16:32:46Z

other/cordic.py

+        return Cordic.fixed_to_float(Cordic.cordic(x, y, 0, CordicMode.VECTOR)["z"])
+
+    @staticmethod
+    def arctan_cordic(y: int) -> float:


Please provide descriptive name for the parameter: y

algorithms-keeper · 2022-10-21T16:32:47Z

other/cordic.py

+        return Cordic.fixed_to_float(Cordic.cordic(1, y, 0, CordicMode.VECTOR)["z"])
+
+    @staticmethod
+    def rot_decision(mode: CordicMode, value: int) -> bool:


As there is no test file in this pull request nor any test function or class in the file other/cordic.py, please provide doctest for the function rot_decision

algorithms-keeper · 2022-10-21T16:32:47Z

other/cordic.py

+            return not flag
+
+    @staticmethod
+    def cordic(x: int, y: int, z: int, mode: CordicMode) -> dict:


As there is no test file in this pull request nor any test function or class in the file other/cordic.py, please provide doctest for the function cordic

Please provide descriptive name for the parameter: x

Please provide descriptive name for the parameter: y

Please provide descriptive name for the parameter: z

algorithms-keeper · 2022-10-21T16:32:47Z

other/cordic.py

+        return result
+
+    @staticmethod
+    def fixed_to_float(fixed: int) -> float:


As there is no test file in this pull request nor any test function or class in the file other/cordic.py, please provide doctest for the function fixed_to_float

algorithms-keeper · 2022-10-21T16:32:47Z

other/cordic.py

+        return fixed / SHIFT_BASE
+
+    @staticmethod
+    def float_to_fixed(flt: float) -> int:


As there is no test file in this pull request nor any test function or class in the file other/cordic.py, please provide doctest for the function float_to_fixed

CaedenPH · 2022-10-21T17:57:51Z

@cclauss Can we get your review please

tianyizheng02 · 2022-10-21T20:28:52Z

@zivdar001matin you should also put Fixes: #{$ISSUE_NO} in your comment rather than just #{$ISSUE_NO} so that GitHub automatically closes the issue when this PR is merged

zivdar001matin · 2022-10-21T20:47:28Z

@zivdar001matin you should also put Fixes: #{$ISSUE_NO} in your comment rather than just #{$ISSUE_NO} so that GitHub automatically closes the issue when this PR is merged

Thanks for your edit suggestion.

tianyizheng02 · 2022-10-21T20:53:21Z

@zivdar001matin you should also put Fixes: #{$ISSUE_NO} in your comment rather than just #{$ISSUE_NO} so that GitHub automatically closes the issue when this PR is merged

Thanks for your edit suggestion.

It should not have backticks around them, otherwise GitHub won't recognize it. It should just look like this:

Fixes: #7490

If it worked, then you should see something like the following on the right panel:

zivdar001matin · 2022-10-21T21:04:09Z

@zivdar001matin you should also put Fixes: #{$ISSUE_NO} in your comment rather than just #{$ISSUE_NO} so that GitHub automatically closes the issue when this PR is merged

Thanks for your edit suggestion.

It should not have backticks around them, otherwise GitHub won't recognize it. It should just look like this:

Fixes: #7490

If it worked, then you should see something like the following on the right panel:

Oops x)))
Thank you a lot for explaining.

tianyizheng02

Some refactoring suggestions (see comments)
This algorithm belongs more in the maths/ directory than the other/ directory since this is a numerical analysis algorithm
Add doctests to all functions

tianyizheng02 · 2023-08-03T06:37:37Z

other/cordic.py

This algorithm belongs in the maths/ directory, IMO

tianyizheng02 · 2023-08-03T06:50:25Z

other/cordic.py

+        if value < 0:
+            flag = True
+        else:
+            flag = False
+
+        if mode == CordicMode.ROTATION:
+            return flag
+        else:
+            return not flag


Notice that this function only returns true when value < 0 and mode == CordicMode.ROTATION are both the same value:

value < 0 mode == CordicMode.ROTATION rot_decision

True True True

True False False

False True False

False False True

This makes sense:

If we're in rotation mode, then we just return the flag (meaning that we return true when the flag is also true).

If we're not in rotation mode, then we negate the flag (meaning that we return true when the flag is also false).

Thus, you can simplify the logic to just this:

Suggested change

if value < 0:

flag = True

else:

flag = False

if mode == CordicMode.ROTATION:

return flag

else:

return not flag

return (value < 0) == (mode == CordicMode.ROTATION)

tianyizheng02 · 2023-08-03T06:51:38Z

other/cordic.py

+            return not flag
+
+    @staticmethod
+    def cordic(x: int, y: int, z: int, mode: CordicMode) -> dict:


Please specify the types within the dict

tianyizheng02 · 2023-08-03T06:52:13Z

other/cordic.py

+        x = x << SHIFT
+        y = y << SHIFT
+        z = z << SHIFT


Suggested change

x = x << SHIFT

y = y << SHIFT

z = z << SHIFT

x <<= SHIFT

y <<= SHIFT

z <<= SHIFT

tianyizheng02 · 2023-08-03T06:56:01Z

other/cordic.py

+            if mode == CordicMode.ROTATION:
+                flag = Cordic.rot_decision(mode, z)
+            else:
+                flag = Cordic.rot_decision(mode, y)


Suggested change

if mode == CordicMode.ROTATION:

flag = Cordic.rot_decision(mode, z)

else:

flag = Cordic.rot_decision(mode, y)

rot_check_val = z if mode == CordicMode.ROTATION else y

flag = Cordic.rot_decision(rot_check_val)

Feel free to choose a better variable name, but the idea still stands

tianyizheng02 · 2023-08-03T06:56:30Z

other/cordic.py

+                x = x + (y >> i)
+                y = y - (x_temp >> i)
+                z = z + ELEM_ANGLE[i]


Suggested change

x = x + (y >> i)

y = y - (x_temp >> i)

z = z + ELEM_ANGLE[i]

x += (y >> i)

y -= (x_temp >> i)

z += ELEM_ANGLE[i]

tianyizheng02 · 2023-08-03T06:56:56Z

other/cordic.py

+                x = x - (y >> i)
+                y = y + (x_temp >> i)
+                z = z - ELEM_ANGLE[i]


Suggested change

x = x - (y >> i)

y = y + (x_temp >> i)

z = z - ELEM_ANGLE[i]

x -= (y >> i)

y += (x_temp >> i)

z -= ELEM_ANGLE[i]

tianyizheng02 · 2023-08-03T06:59:04Z

other/cordic.py

+    VECTOR = 1
+
+
+class Cordic:


Since "CORDIC" is usually written in all caps, could we make it all caps in class and function names here?

I am not a fan of this request. I would rather stick with https://beta.ruff.rs/docs/rules/#pep8-naming-n

tianyizheng02 · 2023-08-03T07:02:05Z

other/cordic.py

+
+class Cordic:
+    @staticmethod
+    def cos_cordic(theta: int) -> float:


Could we make numerical variables float or int | float rather than just int?

If so then some of our tests need to use float input.

tianyizheng02 · 2023-08-03T07:03:17Z

other/cordic.py

+        )
+
+    @staticmethod
+    def arctan_div_cordic(x: int, y: int) -> float:


For functions that pass in both the x- and y-coords of a vector, why not pass in the vector as a tuple?

cclauss · 2023-08-03T08:10:58Z

other/cordic.py

+ELEM_ANGLE = [
+    2949120,
+    1740967,
+    919789,
+    466945,
+    234379,
+    117304,
+    58666,
+    29335,
+    14668,
+    7334,
+    3667,
+    1833,
+    917,
+    458,
+    229,
+    115,
+    57,
+    29,
+    14,
+    7,
+    4,
+    2,
+    1,
+]


WIll this work? Tuples take less memory than lists and they are a signal to the reader that the contents will not change at runtime.

Suggested change

ELEM_ANGLE = [

2949120,

1740967,

919789,

466945,

234379,

117304,

58666,

29335,

14668,

7334,

3667,

1833,

917,

458,

229,

115,

57,

29,

14,

7,

4,

2,

1,

]

ELEM_ANGLE = (

2949120,

1740967,

919789,

466945,

234379,

117304,

58666,

29335,

14668,

7334,

3667,

1833,

917,

458,

229,

115,

57,

29,

14,

7,

4,

2,

1,

)

Add CORDIC

080fa37

algorithms-keeper bot added require descriptive names This PR needs descriptive function and/or variable names require tests Tests [doctest/unittest/pytest] are required labels Oct 21, 2022

algorithms-keeper bot reviewed Oct 21, 2022

View reviewed changes

algorithms-keeper bot added awaiting reviews This PR is ready to be reviewed tests are failing Do not merge until tests pass labels Oct 21, 2022

CaedenPH reviewed Oct 21, 2022

View reviewed changes

other/cordic.py Outdated Show resolved Hide resolved

other/cordic.py Outdated Show resolved Hide resolved

Apply suggestions from code review

b143bcd

Co-authored-by: Caeden Perelli-Harris <[email protected]>

algorithms-keeper bot reviewed Oct 21, 2022

View reviewed changes

algorithms-keeper bot removed the tests are failing Do not merge until tests pass label Oct 21, 2022

tianyizheng02 requested changes Aug 3, 2023

View reviewed changes

cclauss reviewed Aug 3, 2023

View reviewed changes

cclauss closed this Sep 30, 2023

Apr	MAY	Jun
	21
2024	2025	2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added CORDIC algorithm #7493

Added CORDIC algorithm #7493

zivdar001matin commented Oct 21, 2022 •

edited

Loading

algorithms-keeper bot left a comment

algorithms-keeper bot Oct 21, 2022

algorithms-keeper bot Oct 21, 2022

algorithms-keeper bot Oct 21, 2022

algorithms-keeper bot Oct 21, 2022

algorithms-keeper bot Oct 21, 2022

algorithms-keeper bot Oct 21, 2022

CaedenPH left a comment

algorithms-keeper bot left a comment

algorithms-keeper bot Oct 21, 2022

algorithms-keeper bot Oct 21, 2022

algorithms-keeper bot Oct 21, 2022

algorithms-keeper bot Oct 21, 2022

algorithms-keeper bot Oct 21, 2022

algorithms-keeper bot Oct 21, 2022

CaedenPH commented Oct 21, 2022

tianyizheng02 commented Oct 21, 2022

zivdar001matin commented Oct 21, 2022

tianyizheng02 commented Oct 21, 2022

zivdar001matin commented Oct 21, 2022

tianyizheng02 left a comment

tianyizheng02 Aug 3, 2023

tianyizheng02 Aug 3, 2023

tianyizheng02 Aug 3, 2023

tianyizheng02 Aug 3, 2023

tianyizheng02 Aug 3, 2023

tianyizheng02 Aug 3, 2023

tianyizheng02 Aug 3, 2023

tianyizheng02 Aug 3, 2023

cclauss Aug 3, 2023

tianyizheng02 Aug 3, 2023

cclauss Aug 3, 2023 •

edited

Loading

tianyizheng02 Aug 3, 2023

cclauss Aug 3, 2023

`value < 0`	`mode == CordicMode.ROTATION`	`rot_decision`
`True`	`True`	`True`
`True`	`False`	`False`
`False`	`True`	`False`
`False`	`False`	`True`

Added CORDIC algorithm #7493

Added CORDIC algorithm #7493

Conversation

zivdar001matin commented Oct 21, 2022 • edited Loading

Describe your change:

Checklist:

algorithms-keeper bot left a comment

Choose a reason for hiding this comment

🔗 Relevant Links

Repository:

Python:

Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.

algorithms-keeper actions can be triggered by commenting on this PR:

algorithms-keeper bot Oct 21, 2022

Choose a reason for hiding this comment

algorithms-keeper bot Oct 21, 2022

Choose a reason for hiding this comment

algorithms-keeper bot Oct 21, 2022

Choose a reason for hiding this comment

algorithms-keeper bot Oct 21, 2022

Choose a reason for hiding this comment

algorithms-keeper bot Oct 21, 2022

Choose a reason for hiding this comment

algorithms-keeper bot Oct 21, 2022

Choose a reason for hiding this comment

CaedenPH left a comment

Choose a reason for hiding this comment

algorithms-keeper bot left a comment

Choose a reason for hiding this comment

🔗 Relevant Links

Repository:

Python:

Automated review generated by algorithms-keeper. If there's any problem regarding this review, please open an issue about it.

algorithms-keeper actions can be triggered by commenting on this PR:

algorithms-keeper bot Oct 21, 2022

Choose a reason for hiding this comment

algorithms-keeper bot Oct 21, 2022

Choose a reason for hiding this comment

algorithms-keeper bot Oct 21, 2022

Choose a reason for hiding this comment

algorithms-keeper bot Oct 21, 2022

Choose a reason for hiding this comment

algorithms-keeper bot Oct 21, 2022

Choose a reason for hiding this comment

algorithms-keeper bot Oct 21, 2022

Choose a reason for hiding this comment

CaedenPH commented Oct 21, 2022

tianyizheng02 commented Oct 21, 2022

zivdar001matin commented Oct 21, 2022

tianyizheng02 commented Oct 21, 2022

zivdar001matin commented Oct 21, 2022

tianyizheng02 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cclauss Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zivdar001matin commented Oct 21, 2022 •

edited

Loading

cclauss Aug 3, 2023 •

edited

Loading