symforce-org / symforce

Fast symbolic computation, code generation, and nonlinear optimization for robotics
https://symforce.org
Apache License 2.0
1.44k stars 147 forks source link

Add `to_angle` for the `Rot2` class #358

Closed hflemmen closed 1 year ago

hflemmen commented 1 year ago

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.

hflemmen commented 1 year ago

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

aaron-skydio commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

aaron-skydio commented 1 year ago

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 commented 1 year ago

True, i used black version 23.7.0.

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

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

hflemmen commented 1 year ago

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 commented 1 year ago

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