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

simulation.subsection to include PML #1836

Merged
merged 1 commit into from
Jul 26, 2024
Merged

Conversation

weiliangjin2021
Copy link
Collaborator

@weiliangjin2021 weiliangjin2021 commented Jul 15, 2024

ElectromagneticFieldData.dot to reindex coords for aligning almost identical coordinates

@weiliangjin2021
Copy link
Collaborator Author

weiliangjin2021 commented Jul 16, 2024

One test still errored. So I think this implementation is not robust. I'll try the custom grid idea.

@momchil-flex momchil-flex added 2.7 will go into version 2.7.* .2 labels Jul 16, 2024
@weiliangjin2021
Copy link
Collaborator Author

Please check out the new implementation. CustomGridBoundaries is risky as we don't validate if the coordinate covers exactly the entire simulation domain. So for now I plan not to expose it to external users: it will not be added to doc API, and not added to tidy3d namespace. It can only be used in the imported classmethod.

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.

This will make things so much easier! Thanks @weiliangjin2021

Dont forget changelog

tidy3d/components/grid/grid_spec.py Outdated Show resolved Hide resolved
tidy3d/components/grid/grid_spec.py Outdated Show resolved Hide resolved
tidy3d/components/grid/grid_spec.py Show resolved Hide resolved
tidy3d/components/grid/grid_spec.py Outdated Show resolved Hide resolved
@weiliangjin2021
Copy link
Collaborator Author

Please check out the new implementation. CustomGridBoundaries is risky as we don't validate if the coordinate covers exactly the entire simulation domain. So for now I plan not to expose it to external users: it will not be added to doc API, and not added to tidy3d namespace. It can only be used in the imported classmethod.

Maybe I can validate it in simulation's root validator: sim.simulation_bounds align with the grid coords?

@weiliangjin2021
Copy link
Collaborator Author

weiliangjin2021 commented Jul 18, 2024

@dbochkov-flexcompute now that I added the validator to validate the simulation domain boundaries match the grid boundaries, mode_solver.reduced_simulation_copy will fail the validation because it removes PML layers. Should I add it back?

@dbochkov-flexcompute
Copy link
Contributor

dbochkov-flexcompute commented Jul 18, 2024

@dbochkov-flexcompute now that I added the validator to validate the simulation domain boundaries match the grid boundaries, mode_solver.reduced_simulation_copy will fail the validation because it removes PML layers. Should I add it back?

hmm, not sure if that would resolve the issue. I think we should make CustomGridBoundaries just to match only original sim bounds, without pml, so that pml layers are constructed as usual. That would also avoid an issue when subsection is taken in region of non-uniform grid and that wouldn't have uniform spacing near boundaries like pml. Btw, are coordinates outside of target region discarded? Can't find the place where it is done.

@weiliangjin2021
Copy link
Collaborator Author

hmm, not sure if that would resolve the issue. I think we should make CustomGridBoundaries just to match only original sim bounds, without pml, so that pml layers are constructed as usual.

Well, that can be achieved with CustomGrid. In my test, the grid in original sim bounds with CustomGrid matches exactly. However, once we start to construct pml layers, there is machine precision issue (the grid differ by around 1e-15). That is reason for this CustomGridBoundaries to include the complete grids without any further constructions.

Btw, are coordinates outside of target region discarded? Can't find the place where it is done.

No postprocessing is currently performed. The coordinate must span exactly the simulation domain size.

@weiliangjin2021
Copy link
Collaborator Author

hmm, not sure if that would resolve the issue. I think we should make CustomGridBoundaries just to match only original sim bounds, without pml, so that pml layers are constructed as usual.

Well, that can be achieved with CustomGrid. In my test, the grid in original sim bounds with CustomGrid matches exactly. However, once we start to construct pml layers, there is machine precision issue (the grid differ by around 1e-15). That is reason for this CustomGridBoundaries to include the complete grids without any further constructions.

You are right. There is something wrong in my test setup. I'll refactor it to optionally construct pml layers.

@weiliangjin2021 weiliangjin2021 marked this pull request as draft July 18, 2024 03:45
@weiliangjin2021 weiliangjin2021 marked this pull request as ready for review July 19, 2024 20:53
@weiliangjin2021
Copy link
Collaborator Author

@dbochkov-flexcompute now that I added the validator to validate the simulation domain boundaries match the grid boundaries, mode_solver.reduced_simulation_copy will fail the validation because it removes PML layers. Should I add it back?

hmm, not sure if that would resolve the issue. I think we should make CustomGridBoundaries just to match only original sim bounds, without pml, so that pml layers are constructed as usual. That would also avoid an issue when subsection is taken in region of non-uniform grid and that wouldn't have uniform spacing near boundaries like pml. Btw, are coordinates outside of target region discarded? Can't find the place where it is done.

Now CustomGridBoundaries works almost identical to CustomGrid, except that it takes grid boundaries rather than grid size.

Copy link
Contributor

@dbochkov-flexcompute dbochkov-flexcompute left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for fixing this and making it more robust. Just a few minor comments below

tidy3d/components/grid/grid_spec.py Outdated Show resolved Hide resolved
tidy3d/components/grid/grid_spec.py Outdated Show resolved Hide resolved
tidy3d/components/grid/grid_spec.py Outdated Show resolved Hide resolved
tidy3d/components/simulation.py Outdated Show resolved Hide resolved
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.

couple minor things.

CHANGELOG.md Outdated Show resolved Hide resolved
tidy3d/components/grid/grid_spec.py Outdated Show resolved Hide resolved
tidy3d/components/grid/grid_spec.py Outdated Show resolved Hide resolved
CustomGridBoundaries to precisely replicate grids from a simulation of identical size
@momchil-flex momchil-flex merged commit 85bc872 into develop Jul 26, 2024
9 checks passed
@momchil-flex momchil-flex deleted the weiliang/mode_subsection branch July 26, 2024 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.7 will go into version 2.7.* .2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants