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

Tech-Audit #4: Documentation & Tutorials #196

Closed
TomDonoghue opened this issue Jun 22, 2020 · 5 comments
Closed

Tech-Audit #4: Documentation & Tutorials #196

TomDonoghue opened this issue Jun 22, 2020 · 5 comments
Assignees
Labels
discussion Issue for discussing a topic / idea.

Comments

@TomDonoghue
Copy link
Member

TomDonoghue commented Jun 22, 2020

This issue is part of a 'technical audit' of neurodsp, with @elybrand.

We have a lot of tutorials & documentation as part of this project, all of which is hosted here:
https://neurodsp-tools.github.io/neurodsp/

More specifically, I think this relates to the Tutorial and Glossary sections.

In terms of scoping, we have tried to set it up that we don't try and teach DSP in our documentation (we don't really have the scope to do so). The goal is broadly to demonstrate how to do DSP with our module, for users who we assume already have at least some relevant knowledge.

I think it would be useful to do a quick through of our documentation, checking:

  • Are the technical descriptions we do describe technically correct and do they seem appropriate and well described for the context
  • Are the code & examples we use, and in particular, any parameters that are used good? Basically, a sanity check that we are not accidentally suggesting any bad practice or bad settings that might be problematic.

Sidenote: if you know any good resources for learning DSP that you think might be useful, feel free to mention them, and we can link them from our pages.

For this, scanning through the tutorials might be a good way to get familiar with the module, and you can as always open PRs if you want to propose fixes, but it's also totally fine to just note any sections you are not sure about and we can delegate updates, etc.

@TomDonoghue TomDonoghue added the discussion Issue for discussing a topic / idea. label Jun 22, 2020
@elybrand
Copy link
Contributor

This is admittedly pedantic, but the doctstring inside robust_hilbert() is technically not correct. What you're returning is not the hilbert transform. You're returning the analytic signal, of which the imaginary part is the hilbert transform. It could be confusing if sometime makes a call to robust_hilbert expecting the transform.

@elybrand
Copy link
Contributor

There doesn't appear to be a tutorial for using wavelets. I don't imagine this is a huge priority given the methods paper by Bruns. Bruns equates time-frequency analysis with morelet wavelets to using a Hilbert Transform where the signal is first pre-processed using a filter whose band-pass transfer function (in the frequency domain) is symmetric about its central frequency. Since signals are filtered in [phase, freq, amp]_by_time() using a Hamming window (in the case of FIR), this is certainly the case. However, it's worth noting that currently there is no filtering option in robust_hilbert().

@rdgao
Copy link
Contributor

rdgao commented Jun 26, 2020

This is admittedly pedantic, but the doctstring inside robust_hilbert() is technically not correct. What you're returning is not the hilbert transform. You're returning the analytic signal, of which the imaginary part is the hilbert transform. It could be confusing if sometime makes a call to robust_hilbert expecting the transform.

I have nothing to contribute here, other than to say omfg yes @elybrand.

(I think the intention was just to mimic scipy.signal.hilbert, i.e. the function is called hilbert but should return the analytic signal)

@TomDonoghue
Copy link
Member Author

@elybrand - good catch on the Hilbert stuff! I actually find this a pretty funny error on our side, since Richard has previously done a whole deep dive Hilbert transforms, including that scipy's Hilbert function is not the hilbert transform (http://www.rdgao.com/roemerhasit_Hilbert_Transform/). (I think the lapse here is an old docstring, from Scott-times).

The idea of a tutorial page at least briefly showing our wavelets is something of interest to add (#166) - though perhaps not a huge priority. I think one thing to keep in mind is that once we settle any open discussion points in #165, then adding a tutorial might be a good place to discuss any relevant points we consider important to note. So, I'd say let's keep the option of adding this open.

@TomDonoghue
Copy link
Member Author

This issue / part of the audit is all done. Closing now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Issue for discussing a topic / idea.
Projects
None yet
Development

No branches or pull requests

3 participants