Skip to content

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

Merged
merged 4 commits into from
Dec 3, 2024

Conversation

wagenadl
Copy link
Contributor

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.

Closes #247.

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.
@apdavison
Copy link
Contributor

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.

In [1]: import quantities as p.q.
In [2]: from quantities import unit_registry
In [3]: t = 2 * pq.ms
In [4]: f = 3 * pq.MHz
In [5]: (t * f).rescale(unit_registry['dimensionless']).magnitude
Out[5]: array(6000.)

and

In [6]: t = [1, 2, 3] * pq.ms
In [7]: f = [4, 5, 6] * pq.MHz
In [8]: (t * f).rescale(unit_registry['dimensionless']).magnitude
Out[8]: array([ 4000., 10000., 18000.])

any thoughts from @bjodah, @zm711 or any other recent quantities contributor?

@zm711
Copy link
Contributor

zm711 commented Nov 18, 2024

I think plain is unclear unfortunately. I get the sentiment, though. Maybe something like unit_less? or unitless since dimensionless is already loaded as the unitless value. I agree with Andrew that plain is both unclear in meaning, but also has a slight float/int flavor rather than keeping it as an array. I'm not sure on the name though...

@wagenadl
Copy link
Contributor Author

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 dimensionless constant in the pq module. Would that be confusing to users?

@bjodah
Copy link
Contributor

bjodah commented Nov 18, 2024

Maybe a mouthful, but I would call it .dimensionless_magnitude. As for float vs. int: I think it by default should return whatever dtype the array currently has, be it np.float64 or np.float32 or int64. As for accessing the scalar value, I would use the .item() method which is agnostic to shape (as long as .size == 1):

>>> 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 .item() method (in some relevant docstring perhaps?), rather than adding additional convenience wrappers?

Personally I would avoid making a method a @property if it's not too uncommon that it raises an error.

@zm711
Copy link
Contributor

zm711 commented Nov 18, 2024

Maybe a mouthful, but I would call it .dimensionless_magnitude

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 np.float32 but the final quantities end up being np.float64 just by doing np.array([1,1,1], dtype=np.float32) * 1*pq.s, but that is out of the scope of this PR. Once it has been cast to float64 I think it should be kept there and the user should explicitly cast with as_type.

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.
@wagenadl
Copy link
Contributor Author

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.

@bjodah
Copy link
Contributor

bjodah commented Nov 20, 2024

I'm fine with the current set of changes, and I can see the value of using @property. But I'd like to see a few tests added in some relevant test file (not sure which, perhaps test_methods.py?).

@wagenadl
Copy link
Contributor Author

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.)

Copy link
Contributor

@bjodah bjodah left a 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!

Copy link
Contributor

@zm711 zm711 left a 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.

@wagenadl
Copy link
Contributor Author

wagenadl commented Dec 3, 2024

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.

@apdavison
Copy link
Contributor

GitHub won't run workflows (existing or new) on a PR from a new contributor until this has been approved.

@apdavison apdavison merged commit 93f44a3 into python-quantities:master Dec 3, 2024
39 checks passed
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.

Converting dimensionless quantities back to plain numbers is errorprone
4 participants