Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add to_angle for the Rot2 class #358

Closed
wants to merge 4 commits into from
Closed

Conversation

hflemmen
Copy link
Contributor

I suggest adding a convenience function to get the angle from a 2D rotation. It complements the existing from_angle, and is intuitive to use when working with 2D rotation angles.

Copy link
Member

@aaron-skydio aaron-skydio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, yeah I think this is a good thing to have


This is equivalent to ``to_tangent()[0]``
"""
return sf.atan2(self.z.imag, self.z.real, epsilon=epsilon)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should just call to_tangent to avoid duplicating the implementation, like from_angle does with from_tangent

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, I changed it now.

@hflemmen
Copy link
Contributor Author

hflemmen commented Aug 8, 2023

It seems like it fails the lint checks. Do you have a special linter format?

@aaron-skydio
Copy link
Member

You need a type annotation for epsilon: T.Scalar:

symforce/geo/rot2.py:140: error: Function is missing a type annotation for one or more arguments  [no-untyped-def]

You can run the linter with make lint (it's just black, mypy, and pylint with the configurations in pyproject.toml and pylintrc)

@hflemmen
Copy link
Contributor Author

hflemmen commented Aug 8, 2023

Thank you for the explanation. make lint did not work for me, possibly due to the issue with python 3.10.7, which is mentioned in the test case. I added the scalar type for epsilon.

@aaron-skydio
Copy link
Member

Looks like the thing that's failing now is the autoformat - make format should work fine on py3.10, or you can just copy-paste the suggested change

@hflemmen
Copy link
Contributor Author

hflemmen commented Aug 9, 2023

make format changed a lot of files that I have not touched. Should I commit those as well?

@aaron-skydio
Copy link
Member

No - everything that make format should format is checked by CI - most likely that means you have a different version of black than we use (21.12b0, from setup.py). If you're on the same version of black and see other files changed that would be good to know about though, I'd be curious what the changes are

@hflemmen
Copy link
Contributor Author

True, i used black version 23.7.0.

There are rather many files that are affected, but mostly for the same reasons:

  • Add spaces around ** operators.
  • Remove blank line after :.
  • Remove parenthesis around tuples in for loops. E.g. for (expr, name) in self.override_methods.items(): -> for expr, name in self.override_methods.items():

After revering to the correct version of black, the two latter ones were not undone, so I guess that it accepts both versions.

@hflemmen
Copy link
Contributor Author

Now that all checks have passed, is there more I need to do before the review is passed and it can be merged?

@aaron-skydio
Copy link
Member

aaron-skydio commented Aug 21, 2023

True, i used black version 23.7.0.

There are rather many files that are affected, but mostly for the same reasons:

* Add spaces around `**` operators.

* Remove blank line after `:`.

* Remove parenthesis around tuples in for loops. E.g. `for (expr, name) in self.override_methods.items():` -> `for expr, name in self.override_methods.items():`

After revering to the correct version of black, the two latter ones were not undone, so I guess that it accepts both versions.

Thanks, makes sense

Now that all checks have passed, is there more I need to do before the review is passed and it can be merged?

Nothing more you need to do, I'll merge it

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

Successfully merging this pull request may close these issues.

2 participants