Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
apdavison merged 4 commits intopython-quantities:masterfromwagenadl:plain
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 pqt = 2 * pq.msf = 3 * pq.MHzn = (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 plainnumber 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 6000If 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_registryIn [3]: t = 2 * pq.msIn [4]: f = 3 * pq.MHzIn [5]: (t * f).rescale(unit_registry['dimensionless']).magnitudeOut[5]: array(6000.)

and

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

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

@zm711
Copy link
Contributor

I thinkplain is unclear unfortunately. I get the sentiment, though. Maybe something likeunit_less? orunitless sincedimensionless is already loaded as theunitless value. I agree with Andrew thatplain is both unclear in meaning, but also has a slightfloat/int flavor rather than keeping it as an array. I'm not sure on the name though...

@wagenadl
Copy link
ContributorAuthor

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 thedimensionless constant in thepq module. Would that be confusing to users?

@bjodah
Copy link
Contributor

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

>>>importquantitiesaspq>>>f=42*pq.Hz>>>t=3600*pq.s>>>e=t*f;earray(151200.)*s*Hz>>>e.rescale(pq.dimensionless).magnitude.astype(int).item()151200>>>e2=e.reshape((1,1,1,1,1));e2array([[[[[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

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 ofnp.float32 but the final quantities end up beingnp.float64 just by doingnp.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 tofloat64 I think it should be kept there and the user should explicitly cast withas_type.

bjodah reacted with thumbs up emoji

Renamed the originally proposed “plain” property into“dimensionless_magnitude” as suggested.Improved documentation of the property.Added documentation to the “magnitude” property to clarifythe distinction between the two.
@wagenadl
Copy link
ContributorAuthor

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 commentedNov 20, 2024
edited
Loading

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, perhapstest_methods.py?).

@wagenadl
Copy link
ContributorAuthor

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

@bjodahbjodah 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

@zm711zm711 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
ContributorAuthor

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.

@apdavisonapdavison merged commit93f44a3 intopython-quantities:masterDec 3, 2024
39 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@bjodahbjodahbjodah approved these changes

@zm711zm711zm711 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Converting dimensionless quantities back to plain numbers is errorprone
4 participants
@wagenadl@apdavison@zm711@bjodah

[8]ページ先頭

©2009-2025 Movatter.jp