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

deg2rad unexpected behavior #650

Open
AllenDowney opened this issue Jul 3, 2018 · 4 comments
Open

deg2rad unexpected behavior #650

AllenDowney opened this issue Jul 3, 2018 · 4 comments
Labels
numpy Numpy related bug/enhancement

Comments

@AllenDowney
Copy link

The wrapped version of np.deg2rad does not do what I expect if the argument is a dimensionless Quantity.

Expected: I think a dimensionless argument should be assumed to be in degrees, since that's the normal behavior of this function given a simple number (not a Pint Quantity).

Actual: It treats the argument as if it is in radians:

Example:

angle = 45 * ureg.dimensionless
np.deg2rad(angle)

The result is 45 radians, not 0.78 radians, as I expected.

Is the current behavior as intended?

@hgrecco
Copy link
Owner

hgrecco commented Jul 12, 2018

We have not discussed this as using deg2red and similar is unnecessary with Pint. I open to suggestions in terms on expected behaviour and implementantion.

@AllenDowney
Copy link
Author

For expected behavior, I would think that deg2rad should:

  • If the argument is in degrees, convert to radians (current behavior)
  • If it's already in radians, leave it alone (current behavior)
  • If it's not a Quantity, convert to radians (current behavior)
  • If it's dimensionless, convert to radians (not current behavior)

I think this last one is correct because degrees are dimensionless, so if the argument for this function is dimensionless, we should assume it's degrees (that is, we should assume the caller got it right because we don't have ground to think they're wrong).

For implementation, it looks like you already have a special case for radians:

def __ito_if_needed(self, to_units):
        if self.unitless and to_units == 'radian':
            return

        self.ito(to_units)

Maybe there should also be a special case for degrees?

@hgrecco hgrecco added the numpy Numpy related bug/enhancement label Dec 3, 2019
@hgrecco
Copy link
Owner

hgrecco commented Dec 3, 2019

Revisit after #905

@jthielen
Copy link
Contributor

This issue was left unchanged by #905, but it has been made easier to fix. With that in mind, is @AllenDowney's proposed expected behavior what Pint should go with?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
numpy Numpy related bug/enhancement
Projects
None yet
Development

No branches or pull requests

3 participants