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

Add Spatial frequency, with spectral equivalence support #38

Merged
merged 5 commits into from
Jul 26, 2018

Conversation

ksunden
Copy link
Contributor

@ksunden ksunden commented Jul 25, 2018

I spoke to @ngoldbaum briefly at Scipy2018 about the spectral equivalence and the units my particular sector of spectroscopy uses. In particular we use wavenumbers (1/cm) extensively.

This PR adds a spatial frequency dimension 1 / (length), and then makes the spectral equivalence able to convert to and from spatial_freqency units.

I considered adding wn as a unit (equivalent to 1/cm), but concluded that would likely be better suited to my more specific packages. If the maintainers would like to add it, let me know, perfectly willing to submit such a PR.

@ngoldbaum
Copy link
Member

Hey I'm glad this was easy :)

This looks correct to me but I'd like @jzuhone to take a look as well.

I guess wn means wave-number? I think calling 1/cm wave number might be a tad too domain-specific (as an astronomer that seems a little weird to me).

@ksunden
Copy link
Contributor Author

ksunden commented Jul 25, 2018

Yes indeed wn means wavenumber. Figured it may be too specific, so I can just add a unyt.define_unit('wn', 1/unyt.cm) within my own package when the time comes to replace our homebuilt, not as general unit system.

Wavenumbers are very popular with those who do infrared spectroscopy.
The first thing I tried to get wavenumbers to work was hackily defining it as an energy unit (rather than properly accounting for the dimensions) as it scales linearly with photon energy. That worked, but I do feel this is a better solution as submitted.

The other avenue I considered before diving into the code itself was allowing equivalence translations to take the inverse of any requested unit (as the information to get to length was already there, and spatial frequency is just it's multiplicative inverse).
Such an approach would have the advantage of being able to get things like temporal period out of the "spectral" equivalence.
That seemed more complicated to implement, and I have not thought through implications for other kinds of equivalencies. But It's an idea that would provide an alternate route. I do still think defining a spatial_frequency dimension would be useful in that case, but the translations would be defined differently perhaps.

@ksunden
Copy link
Contributor Author

ksunden commented Jul 25, 2018

I may at some point (especially if I want to use this in theoretical applications) wish to add angular frequencies (both spatial and temporal) to this equivalence as well and complete the cube (with additional offshoot to photon energy):

cube

@jzuhone
Copy link
Contributor

jzuhone commented Jul 25, 2018

Currently at O’Hare with my family catching a plane back to Boston, but i’ll look at this tonight.

@ngoldbaum
Copy link
Member

@jzuhone no rush!

@jzuhone
Copy link
Contributor

jzuhone commented Jul 26, 2018

Looks good—definitely supportive of adding angular frequencies here.

@ngoldbaum
Copy link
Member

Regarding wave numbers, John and I were talking privately over slack about this PR and I realized that there might be a natural way to accomodate them in the library. The reason I was hesitant to call 1/cm wn is that to me a wave number is just one over length, not over over centimeters specifically. I'm also not so hot on the name wn being built in to the library.

What do you think about saying, in dimensions.py:

wave_number = spatial_frequency = 1/length

That should make it possible to do e.g.:

wn = unit_system['wave_number']

And it would return a wavenumber unit appropriate for whatever unit system you've chosen to adopt.

That doesn't have to be part of this pull request, but if you'd be interested in doing a followup that implements this I'd definitely be ok with merging.

In fact I think I'm going to merge this pull request right now along with a quick fix for one of the docstring changes (just expanding the latex formula to include nu_bar).

@ngoldbaum ngoldbaum merged commit d792b96 into yt-project:master Jul 26, 2018
@ksunden ksunden deleted the spatial_frequency branch July 26, 2018 15:26
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.

3 participants