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

Field projection docstrings improvement #959

Merged
merged 1 commit into from
Jun 21, 2023

Conversation

shashwat-sh
Copy link
Contributor

Added more detail to the field projection docstrings based on user feedback. Thanks @tomflexcompute for pointing this out.

Copy link
Contributor

@tomflexcompute tomflexcompute left a comment

Choose a reason for hiding this comment

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

Thanks @shashwat-sh . Looks great. Could you also add some general comments about how the users should set the position and size of the projection monitors?

@shashwat-sh
Copy link
Contributor Author

Thanks @shashwat-sh . Looks great. Could you also add some general comments about how the users should set the position and size of the projection monitors?

@tomflexcompute I added an additional comment on the size. There is already a mention on the position. I don't think we should be too specific though, at some point the users need to understand the physics of the problem enough to know how to set this up correctly for their application. If we make it too specific, I'm worried users will take our suggestion as a "rule", apply it incorrectly, and then blame the solver. @momchil-flex maybe you can take a look too, to see if these suggestions are general enough in your opinion?

Copy link
Contributor

@tomflexcompute tomflexcompute left a comment

Choose a reason for hiding this comment

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

Thanks @shashwat-sh . They look good to me. We definitely want to keep them as general as possible but provide some information at the same time.

description="Whether to enable the far field approximation when projecting fields. "
"If ``True``, terms that decay as O(1/r^2) are ignored, as are the radial components "
"of fields. Typically, this should be set to ``True`` only when the projection distance "
"is much larger than the size of the device being modeled.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

out of curiosity, does the wavelength play into whether far_field_approx is accurate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, in that the observations points should be in the far field of the device - I added a sentence to make this explicit. I didn't specify what number of wavelengths would be considered "far field" because I think that can be application-dependent sometimes.

@tylerflex
Copy link
Collaborator

could you run black? and then feel free to merge, thanks!

tidy3d/components/monitor.py Outdated Show resolved Hide resolved
tidy3d/components/monitor.py Show resolved Hide resolved
@momchil-flex momchil-flex merged commit 42b63e7 into pre/2.3 Jun 21, 2023
0 of 12 checks passed
@momchil-flex momchil-flex deleted the shash/n2f_docs_improvement branch June 21, 2023 16:34
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.

None yet

4 participants