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

Client-side exact projections #575

Merged
merged 1 commit into from
Nov 30, 2022
Merged

Conversation

shashwat-sh
Copy link
Contributor

@shashwat-sh shashwat-sh commented Nov 15, 2022

  • Added in client-side exact projections and associated bool switch in the monitors.
  • Added associated tests
  • Name change: Near2Far -> FieldProjection everywhere
  • Not rebased yet

Backend PR: https://github.com/flexcompute/tidy3d-core/pull/196

@shashwat-sh shashwat-sh requested review from tylerflex and momchil-flex and removed request for tylerflex November 15, 2022 16:09
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 to me!

@momchil-flex
Copy link
Collaborator

momchil-flex commented Nov 16, 2022

I have two comments that are not exactly related to the changes in the current PR, but they're something to think about in line with this ongoing refactoring.

  1. First one is smaller and probably straightforward. It seems to me that monitor here and in other projection data classes should be a Field, and should be mandatory. A number of the in-built properties and methods depend on self.monitor and will break if a monitor is not stored in the data.

  2. I am not a huge fan of how user-side projections currently work, e.g.

            projector = td.FieldProjector.from_near_field_monitors(
                sim_data=sim_data, 
                near_monitors=near_monitors,
                normal_dirs=['-', '+', '-', '+', '-', '+']
            )

            far_field_data = projector.project_fields(proj_monitor)

Specifically, I do not like that in the resulting far_field_data, the information about where the near fields came from (the near_monitors) is lost. You can thus for example end up with different values of the fields in two separate far_field_data instances without understanding why, because the stored self.monitor is the same (the supplied proj_monitor), but different near field data was used.

I thought about this and I realize that it's not straightforward to fix - I kinda understand why it is set up the way it is. But what about having a projection_surfaces list of FieldProjectionSurface instances in the ProjectionMonitorData classes? We can also add a projection_surfaces property to ProjectionMonitor classes which parse the supplied monitor geometry and exclude_surfaces, assigning normals correctly for a box. This could simplify some backend handling too. Then, if the monitor data is created from a server-side ProjectionMonitor, it will just have self.projection_surfaces == self.monitor.projection_surfaces. However, if it was created from near-field monitors user-side, it will store those monitors (and the supplied normal dirs) as projection_surfaces.

@shashwat-sh
Copy link
Contributor Author

shashwat-sh commented Nov 24, 2022

  • Added a property integration_surfaces to SurfaceIntegrationMonitor which extracts surfaces and takes exclude_surfaces into account: see here

  • Consequently was able to simplify backend processing a bit: see here

  • Then specifically for projection monitors, I added projection_surfaces, as suggested by @momchil-flex , which makes use of integration_surfaces: see here. These are also included in the projection data now.

  • Since the monitors now need access to FieldProjectionSurface, I have relocated that class from field_projection.py to monitor.py: see here

  • Added a validator to check the range of ux, uy: here

I think this covers all the remaining loose ends except that precision issue for very far exact projections, but I suggest you review these changes in parallel to my trying to fix that, since it's more on the math / technical side and shouldn't affect the design.

@shashwat-sh shashwat-sh force-pushed the shash/field_proj_3_exact_client branch 2 times, most recently from b55266d to 44c8836 Compare November 29, 2022 21:40
- name change near2far to field projection and associated changes and additions to tests

- made monitors mandatory in field projection data

- integration surfaces and projection surfaces added - proj monitors now store projection surfaces

- add a warning for exact projections for large distances

- updated large distance handling and some docstrings
@shashwat-sh shashwat-sh force-pushed the shash/field_proj_3_exact_client branch from 44c8836 to 5f43696 Compare November 30, 2022 20:03
@shashwat-sh shashwat-sh merged commit 070f3d4 into develop Nov 30, 2022
@tylerflex tylerflex deleted the shash/field_proj_3_exact_client branch May 16, 2023 14:21
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