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

Add notebook demonstrating equivalence of equiangular samplings #278

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jasonmcewen
Copy link
Contributor

No description provided.

Copy link
Collaborator

@matt-graham matt-graham left a comment

Choose a reason for hiding this comment

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

I've added some comments. Ideally I would say if we want to include this as a tutorial notebook there should be a bit more explanatory text about what the various checks / asserts in the notebook are showing and where the expressions for computing the samples points come from. For this to appear in the documentation I believe a corresponding link file needs to be added to docs/tutorials.

"source": [
"# Equivalence of various equiangular samplings\n",
"\n",
"In this demo we show the equivalence between Fejer/Clenshaw-Curtis sampling and other equiangular sampling schemes, including MW, MWSS and DH."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"In this demo we show the equivalence between Fejer/Clenshaw-Curtis sampling and other equiangular sampling schemes, including MW, MWSS and DH."
"In this tutorial we show the equivalence between Fejer / Clenshaw-Curtis sampling and other equiangular sampling schemes, including [McEwen-Wiaux (MW)](https://doi.org/10.1109/TSP.2011.2166394), diametrically symmetric McEwen-Wiaux (MWSS) and [Driscoll-Healy (DH)](https://doi.org/10.1006/aama.1994.1008)."

I think tutorial is better than demo for consistency with terminology we use in documentation and that it's worth expanding the sampling scheme acronyms as it may not be clear what these refer to .

"id": "ae69b19f-c6cb-4f94-a591-946ab9c235fa",
"metadata": {},
"source": [
"Note that Fejer tyle 2 sampling does not include end-points."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"Note that Fejer tyle 2 sampling does not include end-points."
"Note that Fejer type 2 sampling does not include end-points."

"metadata": {},
"outputs": [],
"source": [
"n = L - 1/2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's unclear what n represents here particularly given it appears to be set to be a non-integer value but is used as an argument to numpy.arange. I would say a descriptive variable name would be better or at least some explanatory text.

"metadata": {},
"outputs": [],
"source": [
"thetas_mw_fejer1 = (np.arange(0, n) + 0.5) * np.pi / n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be useful to have a reference for this formula or at least some explanation of how it arises.

"metadata": {},
"outputs": [],
"source": [
"thetas_mwss_fejer2 = np.arange(1, n) * np.pi / n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again without context unclear where this formula comes from / what is being shown here.

@jasonmcewen
Copy link
Contributor Author

Thanks for your comments @matt-graham. Agreed this needs some further clarification. I don't think we should have this as a tutorial notebook as this point (i.e. it shouldn't be rendered in the documentation for now). We can perhaps extend it in future (once we have the additional sampling schemes supported) and then have a more fully featured tutorial notebook. Nevertheless, I'll make some clarifications shortly...

@dipak1munshi
Copy link
Collaborator

Thanks for the invitation to review. I have very straightforward comments.
The notebook looks great. I have some simple comments. Probably not too useful.

  1. Adding some references to the papers where the samplings are introduced
    can be useful to first-time readers.
    2, Some introduction to the notations L, n, .. will be helpful in the notebook.
  2. I am finishing debugging the CC quadrature. I will try to do a similar notebook.
    But, feel free to ignore if the comments are a bit too trivial. Thanks a lot again!

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.

3 participants