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

Allow downsampling of near fields for field projection monitors #1272

Merged
merged 1 commit into from
Nov 22, 2023

Conversation

shashwat-sh
Copy link
Contributor

@shashwat-sh shashwat-sh force-pushed the shash/field_proj_downsampling branch 2 times, most recently from 836bc60 to c8f132e Compare November 22, 2023 16:14
@shashwat-sh shashwat-sh self-assigned this Nov 22, 2023
Copy link
Collaborator

@tylerflex tylerflex 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! just a couple minor comments

] = pydantic.Field(
(1, 1, 1),
title="Spatial interval",
description="Number of grid step intervals at which near fields are recorded for "
Copy link
Collaborator

Choose a reason for hiding this comment

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

So this includes the first and last cell? if there is some discrepancy (the number of cells is not a multiple of interval_space), how is it resolved? For example, if there are cells

A B C D

with interval 2, which will be be included and does it bias to one side (eg. ACD)?

Might be useful to add a brief mention of this if my question makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the same as any other usage of interval_space - yeah, it would be ACD in your example. I can add a mention of this to the other interval_space docstrings too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does that bias the result towards a spatial dimension? Or break symmetry if it was present in the fields?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think if the result is biased because of this, it means the downsampling is way too coarse already, and likely the result wouldn't be correct even if the points were chosen symmetrically. I guess one issue that could arise is if there is symmetry to the fields like you said, where we expect significant cancellations due to interference of the projected fields coming from the "left" and the "right", but even then there's already no guarantee that our Yee grid will be perfectly symmetric, so as long as the near fields are sampled finely enough, I don't think there should be an issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I can try to contrive a backend test for this

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's ok was mostly curious. I think the point about the yee cell being already asymmetric makes some sense. If the user expects symmetric results they should apply symmetry to the simulation already.

pydantic.PositiveInt, pydantic.PositiveInt, pydantic.PositiveInt
] = pydantic.Field(
(1, 1, 1),
title="Spatial interval",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we do capital case for titles ("Spatial Interval") but it seems this is inconsistent across the code base so not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I've applied this to all the titles in monitor.py

"monitor grid is always included. Using values greater than 1 can help speed up "
"server-side far field projections with minimal accuracy loss, especially in cases "
"where it is necessary for the grid resolution to be high for the FDTD simulation, "
"but such a high resolution is unnecessary for the purpose of projecting the recorded "
Copy link
Collaborator

Choose a reason for hiding this comment

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

so the interval space do not propagate to projection_surfaces? which I guess means that this will not be applied for user-side near2far? If I remember correctly, we had a way to downsample these? does it make sense to unify the downsampling somehow (like if interval space specified here, downsample when doing user-side projections too?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For client-side projections, the near fields are recorded via a FieldMonitor which already supports interval_space-based downsampling. Then on top of that, yeah, the user can further downsample based on points per wavelength in the client-side field projector. interval_space in the FieldProjection*Monitor when used client-side will have no effect, because in that case the projection monitor only defines the far field points, not the near field sources. I'm not sure if there's a good way to unify this, because for client-side projection, the FieldProjection*Monitor doesn't even need to be defined at simulation time, but can be defined afterwards just for the projection, in which case there's no way for its interval_space to propagate to the solver

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok makes sense thanks.

@shashwat-sh shashwat-sh merged commit 5cc45f7 into pre/2.5 Nov 22, 2023
13 checks passed
@shashwat-sh shashwat-sh deleted the shash/field_proj_downsampling branch November 22, 2023 23:11
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

3 participants