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

No warning for signal_psd: default min_freq #574

Merged
merged 18 commits into from
Nov 8, 2021
Merged

Conversation

Tam-Pham
Copy link
Member

@Tam-Pham Tam-Pham commented Nov 6, 2021

warning for low-frequency resolution
Address #560

Checklist

Here are some things to check before creating the PR. If you encounter any issues, do let us know :)

  • I have read the CONTRIBUTING file.
  • My PR is targetted at the dev branch (and not towards the master branch).
  • I ran the CODE CHECKS on the files I added or modified and fixed the errors.
  • I have added the newly added features to News.rst (if applicable)

warning for low frequency resolution
@Tam-Pham Tam-Pham linked an issue Nov 6, 2021 that may be closed by this pull request
@DominiqueMakowski
Copy link
Member

But so you agree that currently, by default, in most of the cases: nperseg = int(len(signal) / 2) right?

@DominiqueMakowski
Copy link
Member

why not "sanitize" min_freq to the lowest possible instead of 0.001

@DominiqueMakowski
Copy link
Member

doing: min_freq = max(min_freq, min_possible_freq_given_length_and_SR) and then padding from 0 ?

@DominiqueMakowski
Copy link
Member

We can sanitize min_freq like so, assuming it is 0 by default:

min_freq = max(min_freq, 1/(len(signal) / sampling_rate))

@Tam-Pham
Copy link
Member Author

Tam-Pham commented Nov 8, 2021

But so you agree that currently, by default, in most of the cases: nperseg = int(len(signal) / 2) right?

No 😅 only when the recording length is not sufficiently long to capture at least 2 cycles of min_frequency

I don't know if you are misunderstanding but there is no "impossible frequencies". We are just using the min_frequency to determine the length of overlapping segments when calculating PSD.

@DominiqueMakowski
Copy link
Member

DominiqueMakowski commented Nov 8, 2021

only when the recording length is not sufficiently long

How come i get the warning everytime I use signal_psd then?? even for signals of length a million

@Tam-Pham
Copy link
Member Author

Tam-Pham commented Nov 8, 2021

How come i get the warning everytime I use signal_psd then?? even for signals of length a million

Because if min_frequency is zero (which is adjusted to 0.01), the signal length required for "absolute" high level of good resolution is 4 million 😅

For your use case, you don't have a default value of min_frequency that you want?

@DominiqueMakowski
Copy link
Member

hence the current "adjusted" value is useless since it requires a very rare usecase

All I want is a default behaviour that 1) doesnt' require to manually input min_freq and 2) doesn't print a warning for 99.999% of cases 😅

@Tam-Pham
Copy link
Member Author

Tam-Pham commented Nov 8, 2021

All I want is a default behaviour that 1) doesnt' require to manually input min_freq and 2) doesn't print a warning for 99.999% of cases 😅

What we can do is add a "default" option for min_frequency where min_frequency is determined by optimal nperseg (based on sampling_rate and length of the signal.

@DominiqueMakowski
Copy link
Member

What we can do is add a "default" option

@pull-request-size pull-request-size bot added size/L and removed size/S labels Nov 8, 2021
@codecov-commenter
Copy link

codecov-commenter commented Nov 8, 2021

Codecov Report

Merging #574 (1f42fc1) into dev (0b43f24) will decrease coverage by 2.98%.
The diff coverage is 74.56%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #574      +/-   ##
==========================================
- Coverage   85.80%   82.81%   -2.99%     
==========================================
  Files         187      233      +46     
  Lines        8841    11051    +2210     
==========================================
+ Hits         7586     9152    +1566     
- Misses       1255     1899     +644     
Impacted Files Coverage Δ
neurokit2/complexity/information_mutual.py 77.77% <ø> (ø)
neurokit2/eda/eda_eventrelated.py 100.00% <ø> (+4.08%) ⬆️
neurokit2/eda/eda_findpeaks.py 88.88% <ø> (-0.11%) ⬇️
neurokit2/eog/eog_analyze.py 59.37% <ø> (ø)
neurokit2/eog/eog_eventrelated.py 100.00% <ø> (ø)
neurokit2/events/events_find.py 80.95% <ø> (ø)
neurokit2/microstates/microstates_peaks.py 71.42% <0.00%> (ø)
neurokit2/rsp/rsp_amplitude.py 90.00% <ø> (ø)
neurokit2/rsp/rsp_findpeaks.py 77.77% <ø> (ø)
neurokit2/rsp/rsp_fixpeaks.py 100.00% <ø> (ø)
... and 162 more

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 aaac763...1f42fc1. Read the comment docs.

@DominiqueMakowski
Copy link
Member

@Tam-Pham currently we "normalize" the power in many different places (in signal_psd), can we unify that so that it's done only at one place?

@DominiqueMakowski
Copy link
Member

Why this so nice:

import neurokit2 as nk
import mne
raw.plot_psd(fmin=0, fmax=40., picks=["EEG 050"])

image

And ours so ugly 🥲

channel = nk.mne_channel_extract(raw, what=["EEG 050"]).values
psd = nk.signal_psd(
    channel, sampling_rate=raw.info["sfreq"], show=True, max_frequency=40, method="multitapers"
)

image

@DominiqueMakowski
Copy link
Member

And ours so ugly 🥲

what to do lah

@Tam-Pham
Copy link
Member Author

Tam-Pham commented Nov 8, 2021

Next PR? 🥲

@DominiqueMakowski DominiqueMakowski changed the title Silent option to turn off psd warning No warning for signal_psd: default min_freq Nov 8, 2021
@DominiqueMakowski DominiqueMakowski merged commit dac9c21 into dev Nov 8, 2021
@DominiqueMakowski DominiqueMakowski deleted the fix/psd-warning branch November 8, 2021 23:24
@DominiqueMakowski DominiqueMakowski mentioned this pull request Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

signal_psd() pesky warning
4 participants