Docs update #78
Docs update #78
Conversation
Codecov Report
@@ Coverage Diff @@
## master #78 +/- ##
=======================================
Coverage 98.19% 98.19%
=======================================
Files 9 9
Lines 832 832
=======================================
Hits 817 817
Misses 15 15
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Let me know how I can best be helpful here. Is this ready for a full read-through? |
|
The name change But otherwise, this is ready for a read-through. The big changes are in the |
docs/theory.rst
Outdated
| @@ -16,79 +22,188 @@ The filter shape can be thought of in terms of the kernel of a convolution filte | |||
|
|
|||
| where :math:`f` is the function being filtered, :math:`G` is the filter kernel, and :math:`x'` is a dummy integration variable. | |||
There was a problem hiding this comment.
Per Hussein's comments on our paper, I think we could add a parenthetical statement here saying that our filter with a Gaussian target does not produce exactly the same results as convolution against a Gaussian kernel on the sphere.
| ) | ||
| gaussian_filter | ||
|
|
||
| Once the filter has been constructed, the method ``plot_shape`` can be used to plot the shape of the target filter and the approximate filter. |
There was a problem hiding this comment.
Suggestion: Add a comment saying that the distinction between the target filter and the approximate filter will be discussed below.
| If the number of steps is set too low the filter will not behave as expected: it may not have the right shape or the right length scale. | ||
|
|
||
| Biharmonic steps are counted as 2 steps because their cost is approximately twice as much as a laplacian step. | ||
| The code allows users to set their own ``n_steps``. |
There was a problem hiding this comment.
Somewhere in here we should say that the default number of steps computed by the code for the Taper filter assumes the default transition width. If the user selects a wider transition, then they might want to set a value of n_steps lower than the default to save cost while keeping accuracy. If the user selects a narrower transition then they should set a value of n_steps higher than the default, otherwise the filter won't behave as expected.
There was a problem hiding this comment.
Would it be straightforward to change the code to the effect that it takes the input transition width into account when computing the default n_steps?
There was a problem hiding this comment.
I set the scaling empirically, so I guess I could look for some kind of empirical scaling law that takes the transition width into account. I will put it on my to do list for the next few days.
|
Will do my very best to review this early next week. Thanks for all your work. |
There was a problem hiding this comment.
Finally got around to reviewing this. It's a very impressive update to the documentation, a huge improvement of where we were before. I have not more suggestions beyond what has already been done. Thanks so much @NoraLoose for your work!
I resolved a merge conflict in the Filter class docstring. Once CI passes, I will merge this.
This is a first attempt to restructure the documentation, with the goal of making it more user-friendly. As discussed / suggested in #77, I have made the following changes:
index.rst)basic_filtering.rst)basic-filtering.rst, discuss the filter object and all grid typestheory.rst(essentially the.plot_shapeplots)Still to do: