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

Rotate by radians ? #61

Closed
stuaxo opened this issue Jul 22, 2020 · 9 comments
Closed

Rotate by radians ? #61

stuaxo opened this issue Jul 22, 2020 · 9 comments
Labels

Comments

@stuaxo
Copy link
Contributor

stuaxo commented Jul 22, 2020

In Affine, rotations are always in degrees, but it can sometimes be useful to expose radians.

If you are considering changing the API, would you consider exposing an API that works in radians ?

@mwtoews
Copy link
Contributor

mwtoews commented Jul 23, 2020

Both rotate and shear functions could support an additional use_radians=False argument. See (e.g.) shapely.affinity.rotate where angles in radians are preserved, if required. This enhancement may require a separate cos_sin_rad helper function that "snaps" to a few key angles (maybe inexact "multiples" of pi).

@stuaxo
Copy link
Contributor Author

stuaxo commented Jul 23, 2020

Both rotate and shear functions could support an additional use_radians=False argument. See (e.g.) shapely.affinity.rotate where angles in radians are preserved, if required.

An alternative is to have rotation_radians and make rotation a wrapper of it.

This enhancement may require a separate cos_sin_rad helper function that "snaps" to a few key angles (maybe inexact "multiples" of pi).

Should there be a way to set the snap distance / disable snapping ?

@mwtoews
Copy link
Contributor

mwtoews commented Jul 23, 2020

An alternative is to have rotation_radians and make rotation a wrapper of it.

Whatever is simplest. I'd figure adding an argument would be an easy approach. Keep in mind that the vast majority of users will be using degrees, so I'd prefer to keep this interface simple to see/learn/debug.

Should there be a way to set the snap distance / disable snapping ?

The "snapping" that I mention is to avoid values that are near 0, e.g. these values should all be snapped to zero:

>>> import numpy as np
>>> np.sin(np.pi)
1.2246467991473532e-16
>>> np.sin(2*np.pi)
-2.4492935982947064e-16
>>> np.cos(np.pi/2)
6.123233995736766e-17
>>> np.cos(3*np.pi/2)
-1.8369701987210297e-16

@stuaxo
Copy link
Contributor Author

stuaxo commented Jul 23, 2020

An alternative is to have rotation_radians and make rotation a wrapper of it.

Whatever is simplest. I'd figure adding an argument would be an easy approach. Keep in mind that the vast majority of users will be using degrees, so I'd prefer to keep this interface simple to see/learn/debug.

That makes sense.

Should there be a way to set the snap distance / disable snapping ?

The "snapping" that I mention is to avoid values that are near 0...

Oh, I see - I thought this was about angles close to multiples of 90 degrees.

@mwtoews
Copy link
Contributor

mwtoews commented Jul 23, 2020

Oh, I see - I thought this was about angles close to multiples of 90 degrees.

These are! 90=pi/2, 180=pi, 270=3pi/2, 360=2pi

@stuaxo
Copy link
Contributor Author

stuaxo commented Jul 23, 2020

My fault for not staring at the numbers enough :)

I can imagine an image editor might even want 45 degrees, but this may be YAGNI as I'm not making one right at this moment.

@mwtoews
Copy link
Contributor

mwtoews commented Jul 23, 2020

Hmm, looking around the unit circle there are potentially other angle snap opportunities:

>>> import numpy as np
>>> np.sqrt(2)/2  # exact 45
0.70710678118654757
>>> np.sin(np.pi/4)
0.70710678118654746
>>> np.cos(np.pi/4)
0.70710678118654757
>>> np.sin(np.pi/6)
0.49999999999999994

however, where does it end? I'm not sure how much benefit there could be, so I'm inclined to not do too much snapping. The biggest gain is identifying zero from sin/cos at the 90 degree intervals, otherwise very small coefficient values can creep in affine matrices.

@stuaxo
Copy link
Contributor Author

stuaxo commented Jul 24, 2020

The plotdevice project has an interesting API for rotations, where there is a basis parameter, that can be RADIANS, DEGREES, and PERCENT.

In PERCENT the value is 0.0 to 1.0.

@sgillies
Copy link
Member

sgillies commented Aug 10, 2020

While affine would be more consistent with Python's math module if it took radians instead of degrees, I'm 👎 on accepting both. For applications that are based on radians there is very little extra cost in writing rotation(math.degrees(angle)). @mwtoews I know that I agree to use_radians in https://github.com/Toblerity/Shapely/blob/master/shapely/affinity.py#L132 but I don't want to reproduce that pattern.

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

No branches or pull requests

3 participants