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

Misleading Quantity units with pi #26

Open
GlaserN opened this issue Jan 28, 2021 · 3 comments
Open

Misleading Quantity units with pi #26

GlaserN opened this issue Jan 28, 2021 · 3 comments
Assignees
Labels
can-wait Not working, not urgent discussion Discussions help wanted Extra attention is needed

Comments

@GlaserN
Copy link
Collaborator

GlaserN commented Jan 28, 2021

Is your feature request related to a problem? Please describe.
Inputing a number in radian values needs the unit specifier "Hz"
But inputing a unit in Hz values needs the unit specifier "Hz 2pi"
This is rather counter intuitive and should be changed.

However if you currently use the unit Hz 2pi the input value will be multiplied by 2pi converting from Hz to rad. But it would be more intuitive the other way around to insert a value in Hz with the unit Hz and then transform this value to a radian value, if necessary, e.g. creating the Hamiltonian.

Describe the solution you'd like
Do not transform input value of quantity if 2pi is in the quantity name.
Objects which need a value in radians (e.g. creation of the hamiltonian) should be able to request a value that is in radians and includes a scaling factor dependend on the given unit

Describe alternatives you've considered
Transform an initalized Hz unit to a Hz 2Pi while changing the unit specifier, as well as the value.

Additional context

@GlaserN
Copy link
Collaborator Author

GlaserN commented Jan 28, 2021

An additional problem with the current implementation is that a modification by get_value and set_value is not straightvorward:

v = Qty(3, unit="Hz 2pi")
v.set_value(v.get_value())

leads to an out of bounds error

@shaimach
Copy link
Collaborator

shaimach commented Jan 28, 2021 via email

@lazyoracle lazyoracle added the bug Something isn't working label Jan 28, 2021
@GlaserN
Copy link
Collaborator Author

GlaserN commented Jan 29, 2021

The main point is what should be the unit that invokes the following transformation:

c3/c3/c3objs.py

Lines 74 to 79 in ef95330

if "pi" in unit:
pref = np.pi
if "2pi" in unit:
pref = 2 * np.pi
else:
pref = 1.0

@lazyoracle lazyoracle added the help wanted Extra attention is needed label Feb 2, 2021
@lazyoracle lazyoracle added can-wait Not working, not urgent discussion Discussions and removed bug Something isn't working labels Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
can-wait Not working, not urgent discussion Discussions help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

4 participants