-
Notifications
You must be signed in to change notification settings - Fork 63
Add translated_copy function to ElectromagneticFieldData #2238
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
Conversation
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.
Thanks @caseyflex this looks great!
- The grid_expanded is translated along with the fields. The translated grid_expanded won't in general align with the original Yee grid
- The monitor is translated along with the fields. This was needed for stuff like _diff_area which used the monitor geometry, but it does mean the monitor is not the same as the original simulation monitor. This is related to the comment in _isel in ModeSolverData, where the function is kept private because the _isel is not applied to the monitor settings. There are some differences though: here, at least the translated monitor could correspond to some hypothetical translated simulation, whereas with _isel, it's not always even possible to change the monitor configuration to give you whatever field data you selected.
I think these are fine.
- A rather important one: I updated the logic in _symmetry_update_dict basically to use linear interpolation rather than nearest-neighbor. The reason for this was that it was colocating to cell boundaries, and using nearest-neighbor interpolation meant it was basically choosing arbitrarily between the two neighboring values. It seems more robust and consistent to use linear interpolation for colocation throughout, including here. In particular it is more robust to translations (overlaps being translation-invariant etc.)
- Another robustness improvement: in dot, I changed from _colocated_tangential_fields to _interpolated_tangential_fieldsforfields_other(this was already howouter_dot` had it). This might be a bit slower sometimes due to caching of the former, but the former was not working robustly with translations, again because of numerical grid alignment issues.
Yeah so both of those are hitting an important point. I agree that interpolating would be much more robust and it's nice to enable dot
to work with field datasets which are not necessarily on the same coords. However indeed this was originally done due to performance. I had to put quite some effort in getting our postprocessing time down in cases where e.g. there are mode monitors with many spatial points or at many frequencies or modes. To be fair, I do not know or remember in the end of the day how important each change was. Maybe especially if you add assume_sorted=True
it will actually work well (I think I only figured this out later). Could you test the change in time in computing the dot
product for colocated data (could just be mode_data.dot(mode_data)
) when the data is big? If it's significant, we might want to have some way to also keep the old behavior for performance in our post-processing (e.g. introduce a private _dot
function or something).
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.
Thanks @caseyflex this is super useful. Looks good to me minus @momchil-flex comments about performance. Anecdotally, assume_sorted
has been a bottleneck for autograd too, so might be all that is needed.
8b438b0
to
91d0631
Compare
I did a little bit of benchmarking like you said. In my example problem (which wasn't too big), However I also realized something -- the reason The issue was rather in I updated this function in the tests to respect the value of I also added some tests for this and the mode solver translated dot test you described. I fixed up the colocation for the permittivity monitor data tests too. |
91d0631
to
9761ea6
Compare
9761ea6
to
8f58e8d
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.
Ah nice I think this all looks good now! We actually don't need to modify the symmetry expansion (away from .sel) to achieve the more general behavior of .dot (not requiring field datasets to live on the same grid), which is perfect.
@tomflexcompute I think when you plan an example with the Gaussian beam overlap, you should also showcase this new feature!
This PR solves this issue:
#1526
It should replace this previous PR:
#1916
It allows field data to be translated so that overlaps can be taken between data from different locations.
There are some subtleties:
_diff_area
which used the monitor geometry, but it does mean the monitor is not the same as the original simulation monitor. This is related to the comment in_isel
inModeSolverData
, where the function is kept private because the_isel
is not applied to the monitor settings. There are some differences though: here, at least the translated monitor could correspond to some hypothetical translated simulation, whereas with_isel
, it's not always even possible to change the monitor configuration to give you whatever field data you selected._symmetry_update_dict
basically to use linear interpolation rather than nearest-neighbor. The reason for this was that it was colocating to cell boundaries, and using nearest-neighbor interpolation meant it was basically choosing arbitrarily between the two neighboring values. It seems more robust and consistent to use linear interpolation for colocation throughout, including here. In particular it is more robust to translations (overlaps being translation-invariant etc.)dot
, I changed from_colocated_tangential_fields
to _interpolated_tangential_fieldsfor
fields_other(this was already how
outer_dot` had it). This might be a bit slower sometimes due to caching of the former, but the former was not working robustly with translations, again because of numerical grid alignment issues.