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

Added type of frequency clarification #53

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

KronosTheLate
Copy link

@KronosTheLate KronosTheLate commented Feb 22, 2021

In reading the documentation for fftfreqs, I did not find what I came there for - a clarification about if the frequencies were measured in oscillations/second, or radians/second. From my background (engineering student), I am used to talking about angular frequencies in most cases, and so the fact that normal frequency is returned was not obvious to me, an indication that it might not be to everyone. I have therefore appended the following line in the docs for fftfreq and rfftfreq:
The return values are not to be confused with angular frequency.

I considered specifying the unit of oscillation/second, but I felt like specifying this without adding "as opposed to..." was weird, and when adding it, it is longer and more trailing than this solution. Other suggestions are very welcome

In reading the documentation for `fftfreqs`, I did not find what I came there for - a clarification about if the frequencies were measured in oscillations/second, or radians/second. From my background (engineering student), I am used to talking about angular frequencies in most cases, and so the fact that normal frequency is returned was not obvious to me, indication that it might not be to everyone. I have therefore appended the following line in the docs for `fftfreq` and `rfftfreq`:
`The return values are not to be confused with angular frequency.`

I considered specifying the unit of oscillation/second, but I felt like specifying this without adding "as opposed to..." was weird, and when adding it, it is longer and more trailing than this solution. Other suggestions are very welcome
@codecov
Copy link

codecov bot commented Feb 22, 2021

Codecov Report

Merging #53 (a6c1437) into master (193d035) will decrease coverage by 4.75%.
The diff coverage is n/a.

❗ Current head a6c1437 differs from pull request most recent head ac9264e. Consider uploading reports for the commit ac9264e to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master      #53      +/-   ##
==========================================
- Coverage   52.63%   47.87%   -4.76%     
==========================================
  Files           2        2              
  Lines          95       94       -1     
==========================================
- Hits           50       45       -5     
- Misses         45       49       +4     
Impacted Files Coverage Δ
src/definitions.jl 47.31% <ø> (-4.82%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 193d035...ac9264e. Read the comment docs.

@KronosTheLate
Copy link
Author

friendly bump

@jishnub
Copy link
Member

jishnub commented Jul 26, 2021

Perhaps you may also add that the angular frequencies may be obtained by multiplying the output by 2pi

@KronosTheLate
Copy link
Author

Perhaps you may also add that the angular frequencies may be obtained by multiplying the output by 2pi

Check. I am on my phone, so I could not find the UTF8 symbol for pi, but feel free to insert it id you can. Otherwise I think that 'pi' ia more reliable to display properly on e.g. phones.

@jishnub
Copy link
Member

jishnub commented Jul 27, 2021

Yeah pi is better, and in any case is a valid identifier in Julia. Could you add back ticks to 2pi so that it displays as 2pi? This makes it clear that this is inline code.

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.

2 participants