-
Notifications
You must be signed in to change notification settings - Fork 17
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
fft / inverse fft #12
Conversation
@ubsuny/cp1-24 Anyone interested to review this? |
@zbpetersbuf please use the file names as requested in the homework description! |
so I will be sure to do the following (rename the module preparation.py, and gives the freq in the useful values) only,
|
also I am unsure what this pycache file is, I thought I didn't push this but it appeared regardless, should I delete this? |
@zbpetersbuf so your PR should only contain 2 files:
|
I will review this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- remove files
- change file name
- check functions
- add docstrings
- add unit test
fft_folder/inv_fft.py
Outdated
import math as m | ||
import numpy as np | ||
|
||
def analyze_signal(signal, fs, use_filter, selec_filter): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please call this something like “fft”
also make sure this actually takes a pandas time series. in the moment you assume some fs as a time step. That is not how a pandas time series or the CO2 data is organized. Also you do not need any filters since this is another task
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im aware of this but since I dont know what that data will look like this is a placeholder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also fft is already a function in numpy so I though I wasn't just making those functions, but I can if that's what's being asked of me this is no problem I just need to know
fft_folder/inv_fft.py
Outdated
the modles in the function include liner polynomial cubic and exponential | ||
""" | ||
|
||
def anze_revm_trnd(signal, fs, use_filter, selec_filter): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not part of your homework task. You can leave it but it has to work with the pandas time series
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so I am not removing the overall trend to this data before finding the inv fft?
fft_folder/test_inv_fft.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not a unit test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is just something for myself I am not finished with my task
fft_folder/inv_fft.py
Outdated
""" | ||
inv_fft.py | ||
inverse fast fourie transform | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be renamed to preparation.py
cans should container at least three functions:
- fft
- inverse fft
- calculate frequencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the task was to make my own fft function? and not use the numpy functions?
ok to be clear I am not done with this I apologize for the inconvenience |
I know. I just wanted you to point you in the right direction. |
So the task was to make my own fft, ifft and a function that finds the frequencies? and not use the numpy functions? |
No, use numpy fft and inverse fft inside a function that can use the pandas time series data format |
Please note my comment in #3 (comment) to what your function should take as data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs some changes but merging for now to have a starter for the next functions
fft_folder/preparation.py
Outdated
return np.fft.ifft(matrx) | ||
|
||
def calc_freq(data): | ||
n = len(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only works if your time unit is seconds for returning Hz
fft_folder/preparation.py
Outdated
|
||
def inv_fft(data): | ||
|
||
matrx = np.fft.fft(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don’t need to take the fft first your fft function does that
fft_folder/preparation.py
Outdated
matrx = np.fft.fft(data) | ||
return np.abs(matrx) | ||
|
||
def inv_fft(data): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will have to take a frequency as input
fft_folder/preparation.py
Outdated
""" | ||
import numpy as np | ||
|
||
def fft(data): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you rely on that data is the time data in equidistant time slots. This will fail e.g. for months which are not the same lengths.
fft_folder/test_preparation.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a unit test function
@SchrodingersStruggle merging this for now to have a starter file |
Contents
Reviewer