-
Notifications
You must be signed in to change notification settings - Fork 61
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
[ENH] Incomplete tile warning #245
Conversation
Very interesting idea! I make some comments on the current warning to potential update the description. But then I was playing with it for a moment, and I realized it's actually more than fs & freq - it depends on the total simulated time as well.
Also: I'm a little wary that this warning might trigger a lot, and be annoying.
and it would be annoying to trigger the warning a ton. So: maybe we need to update this warning to make it consider |
Thanks! I thought this was worth consideration since it seemed to spark a long discussion in #223. I didn't realize this was dependent on I can also see this warning being annoying - but I think this is important for spectral analyses? Ideally, it would be better if this was only triggered if I can drop the warning here and move it to notes. |
For your first example, scipy raises warning about defaulting The latest commit moves the warning to a note in the sim_oscillation docstring. |
Hey @ryanhammonds : Yeh, I also don't know that there is any way to trigger the warning from The Because of the n_seconds dependency, I've pushed a commit that suggests a updated phrasing of the note in the function. Also, it feels like even if this is not a continuously checked warning, then we should have functionality available to check this. To add this, I've added a sim util function to get an oscillation definition (in terms of whether the definition will include an integer number of cycles), which uses your original check for |
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.
I like the idea of a separate util. I left one comment about truncation. This doesn't seem like it is always an issue and I'll have to run more sims to figure out when it is - so far though it seems like n_seconds
is only an issue when < 1.
srate_check = (fs/freq).is_integer() | ||
|
||
# Time check: check if signal length matches an integer number of cycles | ||
time_check = (n_seconds * freq).is_integer() |
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.
Is truncation always an issue? This example seems to produce an appropriate spectrum:
n_seconds, fs, freq = 1.99, 1000, 10
print((n_seconds * freq).is_integer())
sig = sim_oscillation(n_seconds, fs, freq)
freqs, powers = compute_spectrum(sig, fs)
plot_power_spectra(freqs, powers)
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.
huh, great catch!
So yeh, this time check isn't super specific. Whether the number of cycles leads to funky PSD is a bit more complex than this check - it depends on the actual spectral estimation that is done (for example, with Welchs, I think it depends on the segment size (nperseg
), and how this relates to the signal, more than the details of the total signal. Because of this, I think we should probably just entirely drop this time check, since I otherwise don't really know how to make it more specific (we can't predict the spectral estimation people will do - and this emphasizes that the details of the resulting PSD are somewhat contextual - not fully determined by the sim'd time series).
So, I feel like I should have realized this earlier - but an important thing to keep in mind here is that the power on/off frequency is not defined solely by the simulation, but also the manner of computing the power spectrum - in particular when we are talking about an integer number of cycles. Consider this sim:
Gives the seemingly wonky power spectrum: However, this is all down to the relationship between the simulated frequency, and the We can check this by updating nperseg:
Which gives the a more exact spectrum: By default, fs and nperseg are often multiples of 10, and so frequencies like 10 Hz divide evenly, and frequencies like 7 do not - but this isn't inherent to the simulation, and not predictable from the simulation definition (n_seconds, fs, freq), without knowing something about the spectral estimation. Which ultimately means - I don't think this 'helper' function or the note are actually well posed - since we can't say anything strictly true about the expected spectrum of the simulated signal based on the signal - what matters is the spectral estimation. So - in the end, I think we should drop this PR. @ryanhammonds - thoughts? Does this make sense? I don't currently see a clear way to integrate this into the codebase without it being heavily caveated. As an alternative, this is all some information that I think would be useful to work into a tutorial / example somewhere - and if you feel you have a handle on it, a quick example that discusses and shows the relationship between spectral estimation and expected power spectra would be quite useful I think. |
This adds a tile warning addressing #223, so noobs like myself aren't confused when a slope in the spectrum results from
sim_oscillation
(i.e. fs/freq is not an int). The slope below use to be smooth but now it produces a decreasing slope of harmonics.