-
Notifications
You must be signed in to change notification settings - Fork 40
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
Fix near2far for 2d simulations #1639
Conversation
Thanks @QimingFlex! I think it should be possible to reuse our monitors as they are. I think you haven't yet familiarized yourself with pydantic validators, which is what we use ubiquitously in the code to ensure the user can only set things the way we want them to. I think that's what we can use here and you should start looking through some examples of those in our code (@dmarek-flex could help if needed). I think what you describe, setting theta = pi/2, works specifically because the simulation 0-dim is set to
I think this can be done with the field data too. We don't necessarily want to introduce whole new objects to store the 2D data. We can use the same, and Etheta and Htheta will just always be zeros. We also don't currently explicitly split 2D sims into TE or TM. Depending on the exciting source, those could even be mixed. So it would just be again a matter of rotating everything from the global reference frame where |
Thanks for the detailed explanation and guidance @momchil-flex. I'll start with the pydantic validators and consult Damian. Your approach to simulations and data transformations is clear and helpful! |
Actually, on second thought I feel like everything should be handled internally. That is to say, there will be no difference in the reference frame used from the user perspective depending on whether the simulation is 2D or 3D. The reason why I'm thinking this may make the most sense is so that users easily get consistent results between 2D and 3D sim setups. For example here I want to be able to just change the simulation size along y to 0, nothing else, and have everything working. That means that in the 2D sim I will still get the far field w.r.t. theta, and theta is defined w.r.t. the z axis. I feel like from a usability perspective this is maybe better than requiring the user to rotate the axes based on the simulation 0-sized-dim? @tomflexcompute any input here? |
Yeah totally agree. |
On the flip side though if someone is doing scattering off a Cylinder they would have to set different settings in the projection monitor (different angles, specifically) depending on which axis is the zero-size dim. Still, I feel like this is the better way. |
36d233f
to
d595088
Compare
Hi @momchil-flex , I just updated the revision. After some thought, I believe it's better to conduct near-to-far in the x-y plane for the following reasons:
In this revision, the simulation size along the z-direction determines whether it's a 2D or 3D simulation. If a 2D simulation is determined, theta is set to pi/2, and the propagation constant is related to the Hankel function. These computations are handled internally. However, I encountered an issue with computing the RCS. It seems related to the monitor, which is a 3D monitor and cannot determine if it's a 2D or 3D simulation. To address this, I introduced radar_cross_section_2d. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I don't think this is a good approach @QimingFlex . An immediate case that comes to mind that is not handled well is a grating coupler. If you look at our grating coupler examples, the natural thing to do would be to compare 2d and 3d simulations where the 0D in the 2d sim is along y.
We could consider enabling this for now just so 2D projections are supported in some way, and make sure we validate things well and explain to users how to use the monitor. But I would prefer if we just handle the general case directly...
@@ -534,6 +559,10 @@ def project_fields( | |||
return self._project_fields_cartesian(proj_monitor) | |||
return self._project_fields_kspace(proj_monitor) | |||
|
|||
def is_3d_simulation(self) -> bool: | |||
"""Determine if the simulation is 2D or 3D based on the size attribute.""" | |||
return self.sim_data.simulation.size[2] != 0.01 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this was set for your specific test? Normally we check if sim.size[index] > 0
. We could also consider if sim.grid.num_cells[index] > 1
to capture more general cases with a single pixel along a direction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the comments, @momchil-flex ! To achieve that goal for a general case, I'm considering how to modify the monitor if we allow 0-dim for every dimension. For example, Ez/Hphi and Hz/Ephi are generated when z is 0-dim, Ey/Hphi and Hy/Ephi are generated when y is 0-dim, and the same applies for x being 0-dim. Therefore, we would need to introduce new fields in the field monitor to consider all these three cases. Is this correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition if sim.grid.num_cells[index] > 1
seems to fit well!
You should use the exact same data structures as for the 3D monitors, i.e. Er, Ephi, Etheta, with the polar coordinates defined w.r.t. the global z axis. For e.g. a 2D sim in the xy-plane then, you would have theta = pi / 2 and what you call Ez and Hphi for a TM sim would actually be Etheta and Hphi. All the other 3D components will be zero. If the plane is oriented along yz, then phi is set to 0, and what would be now Ex/Hphi in a purely 2D case correspond to Ephi/Htheta of the 3D data structure. For an xz plane, phi = pi/2 and Ey/Hphi again correspond to Ephi/Htheta. Try to visualize this and let me know if it makes sense to you. I mean, as I'm writing this I do realize that it sounds convoluted. But like I mentioned above, the nice thing would be that I can just swap 2D and 3D sims without needing to change anything else in my scripts, i.e. take a 3D sim and choose to make one of the axes 0D and compare the projection results directly. I am not sure how much this complicates everything internally. Like, I feel like it should be possible to write the 2D projection as a special case of the 3D projection, and not have to do too much going back and forth between different coordinates. Conceptually, it should be possible to write the Green's function for a 2D sim with arbitrary normal axis as a 3D Green's function in the polar coordinates with a global z axis, i.e. in principle you can still write |
What's the status of this PR? is it for 2.7? |
I just went through the code for both Monitors and MonitorData today and got a better understanding of the monitor classes. What you posted makes sense to me. I will test all three 2D scenarios to ensure we get consistent results. |
It still needs some time to be finished. I'm not sure if it is for 2.7, any ideas? @tomflexcompute @momchil-flex |
If it's too much of a rush, I think we can certainly leave it for a later version release or pre-release. |
Ok, for now I'll tag it for 2.8 and mark as a draft just to focus on the PRs that are in their final stages. |
@momchil-flex I've fixed the code for a general 2D farfield projection and tested it with RCS for all three scenarios in 2d. For a 0-dim along the x-direction, users should set φ = π/2 in the farfield monitor to get the desired farfield. For a 0-dim along the y-direction, users should set φ = 0 . For a 0-dim along the z-direction, users should set θ = π/2. This assumes users are familiar with the standard definition of spherical coordinates. I have one question regarding the RCS calculation, which is a class method of the AbstractFieldProjectionData. In this case, I haven't found a way to determine if it is a 2D or 3D simulation based solely on the data class. Do you have any ideas on this? |
Nice! We should have a Simulation level validator that checks this and explains it to the user if it is not set correctly. Namely, if simulation is 2D, iterate over monitors, and for every field projection monitor make sure that the angles are set correctly. Checking if the simulation is 2D is a bit tricky on the validator level actually as you don't have access to the grid as the Simulation is not yet fully initialized. So it could be just a size == 0 along some direction check. If you want to use
You could add a |
I feel like to avoid confusion we shouldn't merge this before we also fix the server-side projections. Those are much more commonly used anyway. @dmarek-flex now has very good understanding of how they work, maybe you can work together to see what will need to change, at least to scope it to decide whether it will be a lot of effort or not, and we can decide what to do. Note that both the exact and the approximate projections will need to be fixed there. |
I agree. I will prioritize the server-side computation. |
407875b
to
5369ed6
Compare
docs/notebooks
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So actually now the two submodule states here have gone stale. I know you were told that you should do this update, but you should revert it now. The original problem was with the develop
branch, which wasn't up to date. Now it is, and the submodules have progressed further, so if we keep this here your PR will revert them. Generally, we should be fixing the develop
branch when this happens, rather than you updating your submodules in the PR. If only the submodules test breaks but you have not made any changes to the submodules, ignore it.
Let's figure out the revert once everything else is ready, I can help you with that.
@@ -2149,11 +2159,15 @@ def eta(self) -> complex: | |||
return ETA_0 / np.sqrt(eps_complex) | |||
|
|||
@staticmethod | |||
def propagation_phase(dist: Union[float, None], k: complex) -> complex: | |||
def propagation_phase(dist: Union[float, None], k: complex, is_2d_simulation: bool) -> complex: | |||
"""Phase associated with propagation of a distance with a given wavenumber.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is not related to your changes, but propagation_phase
and the docstring seems misleading since this is not just a phase factor. It is an overall normalization factor that contains both phase and amplitude decay due to the propagation. Maybe good to fix?
8d1d3d1
to
4dcae30
Compare
4dcae30
to
b327af6
Compare
Hi @momchil-flex So far, we have fixed the FieldAngleProjectionMonitor for both frontend and backend with far-field approximation. For the remaining work, I propose the following:
|
Sounds good. We already validate 2. for an angle projection monitor. We should validate out 1. if it's not yet supported, i.e. in your validator you should add a check if k-space or cartesian monitor + 2D sim and error out. Regarding k-space, it's conceptually exactly the same as angle, except the angles are defined not on a uniform grid in (theta, phi), but through a uniform grid of (kx, ky). The main difference is that in the k-space definition the angles are frequency dependent as they are basically determined through kx, ky and |k| = omega * n / c through simple trigonometry. Does that clear it up for you? |
Having said that it seems like making the 2D KSpace monitors work should be quite simple, but see if you want to try it right away or keep it for later (and validate them out then). |
Thanks. I have one more question regarding the proj_axis for both Cartesian space and k-space. The docstring says it is normal to the monitor. In the example in the docstrings, which uses a box monitor with proj_axis = 2, how does this proj_axis relate to the monitor? Since the monitor has 6 faces, only 2 of them have normals in the z-direction? |
Let's keep that for later? For now, let's check if k-space or Cartesian monitor + 2D sims is used, and handle any errors accordingly. |
26f4db3
to
791423d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I think this is ready now with one tiny message string change.
tidy3d/components/simulation.py
Outdated
): | ||
raise SetupError( | ||
f"Monitor '{monitor.name}' in 2D simulations is coming soon. " | ||
"Please use FieldProjectionAngleMonitor." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Please use 'FieldProjectionAngleMonitor' instead."
aa09e5c
to
b570189
Compare
|
||
# Check if 3D simulation | ||
if all(size != 0 for size in sim_size): | ||
return val |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is 1D checked now?
b570189
to
c23823f
Compare
@weiliangjin2021 It's validated here within the loop for monitors. |
c23823f
to
1d65d88
Compare
1d65d88
to
2d9e358
Compare
Implemented a preliminary fix for the 'near2far' in 2D. The key differences between 2D and 3D of 'near2far' in math are as follows:
Given these differences, my initial idea was to develop a 2D variant of the FieldProjectionAngleMonitor, omitting the theta parameter and inheriting from the AbstractFieldProjectionMonitor. However, since the AbstractFieldProjectionMonitor is heavily designed for 3D features and depends on theta, it seems a 2D monitor would require a ground-up redesign. My current approach involves a rudimentary adaptation where the 3D monitor is used as 2D by setting theta = pi/2, and I added a 2D version of the far-field computation. This mirrors the 3D code with adjustments for the propagation phase and the RCS coefficient. I am relatively new to contributing to a large project, any comments and suggestions on how to more efficiently integrate these changes would be greatly appreciated.