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

Check minimum computable wavelength in tutorial 1 (Capytaine v2) #288

Merged
merged 3 commits into from
Oct 27, 2023

Conversation

jtgrasb
Copy link
Collaborator

@jtgrasb jtgrasb commented Oct 26, 2023

Description

This PR adds a few lines to tutorial 1 to check the minimum computable wavelength for Capytaine based on the mesh.

Type of PR

  • Bug fix
  • New feature
  • Documentation
  • Other: (specify)

Checklist for PR

Additional details

This Capytaine function ensures a minimum of 4 panels per wavelength. The Nyquist–Shannon sampling theorem says that you need at least 2 data points per wavelength to discretize a signal. https://www.worldscientific.com/doi/abs/10.1142/S0218396X02001401 investigates whether 6 elements per wavelength is sufficient.

@jtgrasb jtgrasb changed the title Check minimum computable wavelength in tutorial 1 Check minimum computable wavelength in tutorial 1 (Capytaine v2) Oct 26, 2023
Copy link
Collaborator

@michaelcdevin michaelcdevin left a comment

Choose a reason for hiding this comment

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

Looks good! Splitting out some of this into a new "Frequency and mesh check" section is a good call too, it makes the tutorial easier to follow.

@@ -862,7 +886,7 @@
"kernelspec": {
"display_name": "wot_dev",
"language": "python",
"name": "python3"
"name": "wecopttool2_0_0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jtgrasb I just noticed this after I approved... this line is the reason the CI is failing on the "Build documentation" check. I think changing this back to "name": "python3" should fix it

@michaelcdevin michaelcdevin merged commit d375ce6 into sandialabs:main Oct 27, 2023
8 checks passed
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