astropy / astropy

Astronomy and astrophysics core library
https://www.astropy.org
BSD 3-Clause "New" or "Revised" License
4.38k stars 1.75k forks source link

Unit of Hz defined wrong? #6433

Open mhvk opened 7 years ago

mhvk commented 7 years ago

@scottransom brought up a question that has come up several times already (most recently, #6032), of converting between Hz and an angular frequency. As defined in our unit system Hz is just 1/s. At first, this seems quite consistent with NIST [1], which list it as such among the derived units. However, just a quick look at wikipedia yields Hz being defined as "one cycle per second" [2]. Of course, this is not the same... And least in the context of our unit system it would mean Hz = cycle / s. But then Hz is also logically used for count rates, etc. It seems part of the problem is that one needs to define what is being counted.

Overall, unlike in #6032, I now feel this is a bug, one that has been there since the start (and in fact is in pynbody, where the unit code came from, as well [3]).

The question is what to do. Redefine Hz as cy/s? But that might break code that now uses it differently. Define a second Hz? Add a different equivalency to help this case?

cc @adrn, @astrofrog

[1] https://physics.nist.gov/cuu/Units/units.html [2] https://en.wikipedia.org/wiki/Hertz [3] https://github.com/pynbody/pynbody/blob/master/pynbody/default_config.ini#L222

astrofrog commented 7 years ago

I personally would be against re-defining it, since there is a lot of potential for breakage. I think that most astronomers will likely be expecting time * frequency to give a dimensionless value, so breaking that would cause issues. We could however have an angular speed equivalency and maybe even enable it by default?

pllim commented 7 years ago

I am also against re-defining it, as it would break wavelength to/from frequency conversion for spectra analysis.

mhvk commented 7 years ago

Thinking just a bit more, there is a real problem here, at least if we want the following both to work (on purpose thinking of pulsar-timing types of expectations):

f = 1*u.Hz
# get the associated period
p = (1/f).to(u.s)
# get the associated angular frequency
omega = f.to(u.rad/u.s)

If we were to redefine Hz=cy/s, one couldn't even get the period anymore (rather, it would be in s/cycle, not in itself crazy, but certainly unexpected). This makes me agree with @astrofrog that we cannot redefine Hz. (and yes, also agreed with @pllim, we want frequency = c/wavelength to hold as well...)

@scottransom - any suggestions? It does seem that the best we could do is to offer an explicit equivalency meant for this. Would that be helpful? (Note that the documentation now tries to explain the situation already [1]; could we add anything? perhaps we should at least add something to the docstring of dimensionless_angles itself)

[1] http://docs.astropy.org/en/latest/units/equivalencies.html#angles-as-dimensionless-units

scottransom commented 7 years ago

@mhvk For PINT we are thinking of defining our own unit. But that is kind of klunky for something so basic. But a custom equivalency is also possible. The current dimensionless_angles one does not do what we want. I've linked this to the PINT developers, hopefully some of them will chime in.

astrofrog commented 7 years ago

I think one thing that could be fixed is the behavior of the dimensionless angle equivalency. Maybe 1 rad should be equivalent to 1/2pi instead of 1?

mhvk commented 7 years ago

No, we cannot change dimensionless_angles, as it would definitely break existing usage. E.g., I use the following all the time:

f = ... cy/s
with u.add_enabled_equivalencies(u.dimensionless_angles()):
    phasor = exp(1j*f*t)

I think the two are simply mutually exclusive: sometimes you have angles and want to treat them as dimensionless (which is OK according to SI), and then 1 rad = 1, and in the case of Hz (and only Hz), one has 1 cycle = 1), but cycle is not really seen as an angle at all, but rather as a countable.

So, concrete suggestion: new equivalency cycles_per_second=(u.cy/u.s, u.Hz) or dimensionless_cycles=(u.cy, None)? (The latter would work everywhere an angle was in place, which may not be wanted.)

randyheydon commented 2 years ago

If I may add another perspective to this longstanding question:

My work is in mechanical vibrations. As an example, I would commonly use this equation to find the natural frequency of a mass-spring system: ω = √(k/m). This comes out in units of 1/s, but must be interpreted as rad/s, but usually I want to present the results in Hz. Currently, the only way to get Hz out of that equation is by converting units twice:

omega = np.sqrt(k / m)
omega = omega.to(u.rad/u.s, equivalencies=u.dimensionless_angles()
omega = omega.to(u.Hz, equivalencies=[(u.cy/u.s, u.Hz)])

I don't see any way to make that conversion correctly in one step other than dividing by 2π. Since the starting units are 1/s, which is already equivalent to Hz, the equivalencies don't have any bearing on the result unless I handle them one after the other. And if I have to treat frequencies differently from every other unit, sooner or later I'm going to slip up and silently get a wrong result.

So long story short, I think the only good solution is to define a second Hz. I already do that in my work, adding Hz = u.def_unit('Hz', u.cycle/u.s) at the start. That conflicts with Astropy's Hz so I can't do lookups by name, but it's still good enough for my needs. But maybe those of you more knowledgeable in Astropy's internals can think of a way to deal with that.

mhvk commented 2 years ago

Yes, there is no better route because of Hz=1/s in astropy. I've been wondering about a system similar to how we deal with electron charge in cgs (which has multiple definitions; esu and emu). But it needs some thought...

mhvk commented 2 years ago

One option might be for Hz to be its own unit, but carry a built-in equivalency that by default is to 1/s but can be changed to cycle/s.

maxnoe commented 1 year ago

The SI brochure is very clear about this. Hz = s⁻¹ and there is no unit for "counts" or "cycles" in the SI.

That's how the SI is defined from the seven defining constants.

Section 2.2: Definition of the SI according to Hz = s⁻¹

and in Section 2.3.4:

Note that in some countries, frequency values are conventionally expressed using “cycle/s” or “cps” instead of the SI unit Hz, although “cycle” and “cps” are not units in the SI.

This is a pet pieve of mine:

But then Hz is also logically used for count rates, etc. It seems part of the problem is that one needs to define what is being counted.

No! Hertz is not a valid unit for this. It's the frequency of something periodic. The SI brochure is also very clear:

Section 2.3.4 Derived Units
The SI unit of frequency is hertz, the SI unit of angular velocity and angular frequency is radian per second, and the SI unit of activity is becquerel, implying counts per second. Although it is formally correct to write all three of these units as the reciprocal second, the use of the different names emphasizes the different nature of the quantities concerned.

and

The hertz shall only be used for periodic phenomena and the becquerel shall only be used for stochastic processes in activity referred to a radionuclide.

https://www.bipm.org/documents/20126/41483022/SI-Brochure-9-EN.pdf