Skip to content

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

Merged
merged 1 commit into from
Feb 17, 2025

Conversation

caseyflex
Copy link
Contributor

@caseyflex caseyflex commented Feb 11, 2025

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:

  • 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.
  • 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.

@caseyflex caseyflex linked an issue Feb 11, 2025 that may be closed by this pull request
Copy link
Collaborator

@momchil-flex momchil-flex left a 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).

Copy link
Collaborator

@yaugenst-flex yaugenst-flex left a 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.

@caseyflex caseyflex force-pushed the casey/translatefield branch from 8b438b0 to 91d0631 Compare February 12, 2025 23:31
@caseyflex
Copy link
Contributor Author

@momchil-flex

I did a little bit of benchmarking like you said. In my example problem (which wasn't too big), sel took about .14 seconds, while my new interp code took about .22 seconds. Adding assume_sorted helped a bit, taking about .19 seconds.

However I also realized something -- the reason sel made sense here is that we are assuming that monitor.colocate agrees with the actual colocation of the field data. For example, if monitor.colocate = True, then the field data (unexpanded) should already be colocated, so in this symmetry expansion when we sel at the colocation coords (cell boundaries), it shouldn't be doing any further interpolation.

The issue was rather in tests/test_data/test_data_arrays.py::get_xyz. The field data was being constructed without colocation, regardless of the value of monitor.colocate. So this was actually violating the assumption in symmetry_expanded_copy, which was therefore doing additional zeroth-order interpolation.

I updated this function in the tests to respect the value of monitor.colocate. I changed _symmetry_update_dict to use the original sel implementation if no interpolation is needed; if interpolation is needed, then it uses the less efficient implementation but gives a warning (since this indicates incorrect setup of data / monitor pairing). Alternatively, we could just go back to the sel implementation entirely, but I thought it would be helpful to see if there was ever such a mismatch through a warning (and have a more accurate fallback interpolation). Another alternative would be to raise an error here instead of the graceful fallback + warning.

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.

@caseyflex caseyflex force-pushed the casey/translatefield branch from 91d0631 to 9761ea6 Compare February 12, 2025 23:57
@caseyflex caseyflex force-pushed the casey/translatefield branch from 9761ea6 to 8f58e8d Compare February 13, 2025 00:14
Copy link
Collaborator

@momchil-flex momchil-flex left a 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!

@momchil-flex momchil-flex merged commit 63c1c3d into pre/2.8 Feb 17, 2025
15 checks passed
@momchil-flex momchil-flex deleted the casey/translatefield branch February 17, 2025 13:23
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.

Translation of field data for easier overlap computation
3 participants