-
Notifications
You must be signed in to change notification settings - Fork 75
Added "plain" property to quantity #248
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
Conversation
The new "plain" property returns a copy of the quantity as a plain number provided that the quantity is dimensionless. For example: import quantities as pq t = 2 * pq.ms f = 3 * pq.MHz n = (t*f).plain # n will be 6000 If the quantity is not dimensionless, a conversion error is raised.
Hi @wagenadl - many thanks for this. I think this is potentially useful, although I'm not sure "plain" is the appropriate name, since what the current implementation returns is an array, not a float or int.
and
any thoughts from @bjodah, @zm711 or any other recent quantities contributor? |
I think |
Hi @apdavison et al., Thank you for your encouraging comments. Would it be better to return float/int in case of unsized arrays? (Just as the comparison operators do?) I certainly am open to naming suggestions. A name that makes it clear that the conversion is safe (i.e., raises an error if the quantity is not dimensionless) would be ideal. Strictly speaking, it would not be impossible to name the property “dimensionless” as it lives in a different namespace from the |
Maybe a mouthful, but I would call it >>> import quantities as pq
>>> f = 42*pq.Hz
>>> t = 3600*pq.s
>>> e = t*f; e
array(151200.) * s*Hz
>>> e.rescale(pq.dimensionless).magnitude.astype(int).item()
151200
>>> e2 = e.reshape((1,1,1,1,1)); e2
array([[[[[151200.]]]]]) * s*Hz
>>> e2.rescale(pq.dimensionless).item() # note that we drop unit even without .magnitude (problematic unless dimensionless)
151200.0 If the item() call is to be included in the convenience property method, I think it should also be reflected in the name, but at some point it might be better just to educate the user about the existence of the numpy Personally I would avoid making a method a |
I'm cool with this. I would prefer explicit in this type of situation rather than a name that is unclear. This is the magnitude of the array after correcting for canceling the units (if possible). So +1 for that naming unless Andrew has a reason to be really against it. For scalar I also agree with @bjodah in that I don't see why this can't (read ought to be) be the user's responsibility to pull out the values they want and set the dtype with numpy. My only concern (which is slight) is that as of numpy 2.0 arrays are always upcast even if not necessary (this broke some testing on neo) where we had an array of |
Renamed the originally proposed “plain” property into “dimensionless_magnitude” as suggested. Improved documentation of the property. Added documentation to the “magnitude” property to clarify the distinction between the two.
I really like the name "dimensionless_magnitude" as it makes the connection to "magnitude" so clear. I updated my PR. I have also improved the documentation. I agree that it is not ideal to have properties generate errors, but there is also a strong argument to be made that it could confuse users if "dimensionless_magnitude" requires calling syntax whereas "magnitude" doesn't. Do any of you have opinions on which argument should carry the day? Is it better in the end to have "dimensionless_magnitude" be a property (for symmetry with "magnitude") or a method (to avoid raising exceptions in a property getter)? I welcome your thoughts. |
I'm fine with the current set of changes, and I can see the value of using |
I added three basic tests to test_methods.py and confirmed that they passed. (I also verified that they failed if I temporarily put in incorrect numbers.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good by me too.
Anything else I should do before this can be merged? To be clear, I do not know what the “1 workflow awaiting approval” thing is about that github places on this page. I did not add/change/remove any github workflows in this pull request. |
GitHub won't run workflows (existing or new) on a PR from a new contributor until this has been approved. |
The new "plain" property returns a copy of the quantity as a plain number provided that the quantity is dimensionless. For example:
If the quantity is not dimensionless, a conversion error is raised.
Closes #247.