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

Factoring the Gaussian - Part 3 #134

Merged

Conversation

NoraLoose
Copy link
Member

This PR makes a few minor tweaks to code and documentation related to factoring the Gaussian.

  • In the Filter Class, distinguish between self.filter_scale and self.filter_scale_iterated. As a result, the string representation of the filter can give back the original filter scale (self.filter_scale) that the user put in, rather than the smaller filter scale associated with the constituent filters. I think this makes it more intuitive.
  • I added a few code snippets to factored_gaussian.rst to exemplify the factoring.
  • I added a few clarifying sentences and comments to the numerical instability example notebook.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@@ -421,14 +423,14 @@ def plot_shape(self, ax=None):
linewidth=4,
)
ax.axvline(
2 * np.pi / self.filter_scale,
2 * np.pi / self.filter_scale_iterated,
Copy link
Member

@iangrooms iangrooms Feb 7, 2022

Choose a reason for hiding this comment

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

On the lines above, the target that is plotted is F(x) ** self.n_iterations and the approximation that is plotted is np.polynomial.chebyshev.chebval(x, self.filter_spec.p) ** self.n_iterations. For these plots I think we want the vertical line to be at filter_scale not filter_scale_iterated right?

(Edit: Same comment applies to the zoomed-in plot below)

@iangrooms
Copy link
Member

OK, looks good to me. Thanks @NoraLoose!

@iangrooms iangrooms merged commit bad651d into ocean-eddy-cpt:master Feb 7, 2022
@NoraLoose
Copy link
Member Author

Thanks Ian!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants