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

[DOC] - Update filter tutorial(s) #232

Merged
merged 9 commits into from
Apr 13, 2021
Merged

[DOC] - Update filter tutorial(s) #232

merged 9 commits into from
Apr 13, 2021

Conversation

TomDonoghue
Copy link
Member

@TomDonoghue TomDonoghue commented Dec 31, 2020

This PR updates the filter documentation / tutorials, relating to #229.

@ryanhammonds : following the issue, can you have a go working on this PR? I would suggest digging into the linked papers to guide some extra detail that can be added. If you can organize the new layout, add some detail and extend and you think is needed, I can come back to it after and edit. Thanks!

@TomDonoghue TomDonoghue added the 2.2 Updates to go into a 2.2.0 release label Dec 31, 2020
@neurodsp-tools neurodsp-tools deleted a comment from codecov-io Dec 31, 2020
@neurodsp-tools neurodsp-tools deleted a comment from codecov-io Jan 15, 2021
@ryanhammonds
Copy link
Member

Hi @TomDonoghue, I still have more work to do on this PR, but I made a first attempt at an FIR tutorial. The Widmann 2015 recommendations are in included in the tutorial, so #235 has been squashed into 48a688c.

@neurodsp-tools neurodsp-tools deleted a comment from codecov-io Jan 15, 2021
@TomDonoghue
Copy link
Member Author

I had a quick look at this one. It seems that the math isn't rendering properly? Also, I'm not sure it's particularly needed to get into the details of the math on this. We can't fully cover the technicalities of filters here, and I think more code examples would be better. Also, more general description, introducing, for example what an 'FIR' filter, in particular, is, and the different passtypes (though that might be in a more a different tutorial).

Also, in terms of organizing the tutorials, I'm thinking maybe:

  • 0: filtering in general (introduce passtypes)
  • 1: FIR (introduce specifics of FIR filters)
  • 2: IIR (introduce specifics of IIR filters)

@ryanhammonds
Copy link
Member

I'll remove the math - it helped me understand FIR filters and thought it may be useful to others. I agree with the organization. That was the plan I had in mind too. I'm still working on the IIR tutorial but it'll be finished soon.

@TomDonoghue
Copy link
Member Author

@ryanhammonds - if you found the math to be helpful to explain what's up with filters, then maybe we should keep it! I found it hard to get a sense of when it wasn't rendering - but when it prints properly, it might be useful, so feel free to keep it for now!

@ryanhammonds
Copy link
Member

ryanhammonds commented Jan 21, 2021

Sounds good! The math is fixed by changing ..math to .. math. I have this locally but will push soon.

Base automatically changed from master to main January 25, 2021 21:17
@neurodsp-tools neurodsp-tools deleted a comment from codecov-io Jan 26, 2021
@ryanhammonds ryanhammonds changed the title [WIP/DOC] - Update filter tutorial(s) [DOC] - Update filter tutorial(s) Jan 29, 2021
@neurodsp-tools neurodsp-tools deleted a comment from codecov-io Jan 29, 2021
@TomDonoghue TomDonoghue added the documentation Tasks related to documentation materials & code docs. label Mar 28, 2021
@TomDonoghue
Copy link
Member Author

Okay - I finally came back around to this one. The stuff you added and organized looks great @ryanhammonds, and I also came around to keeping the math! I kept pushing on it a bit, with the main theme of organizing what's where, and avoiding duplication - so the I split out more stuff from the original tutorial, added a fourth section for "filter checks", and edited from there, trying to edit in more information as I could. Thanks for doing a push on this, it really helped to have a big chunk of this done.

This should be ready to merge, I think. @ryanhammonds - when you get a chance, if you could do a quick scan for any typos / bugs / or word quirks, that would be great. If it looks good, we can merge.

Copy link
Member

@ryanhammonds ryanhammonds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good! I'm pushing commit to correct a few typos I found. One thing we may want to do later is add a filter report section to the end of the "checks" tutorial once #235 is merged.

@TomDonoghue
Copy link
Member Author

Awesome, thanks for the check @ryanhammonds - the fixes look great!

And yeh, we should definitely update this to reflect the Filter Report once #235 is ready, I'm going to try to get to that soon.

Merging this in now!

@TomDonoghue TomDonoghue merged commit 702fff9 into main Apr 13, 2021
@TomDonoghue TomDonoghue deleted the filter_docs branch April 13, 2021 00:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.2 Updates to go into a 2.2.0 release documentation Tasks related to documentation materials & code docs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants