Various improvements to EME solver.

[caseyflex]

Summary of changes:

• Adds EMEFreqSweep with freq_scale_factors
• Adds mode solver relative to existing basis via _data_on_yee_grid_relative
• Changes mode solver eigenvalue formula in eigs_to_effective_index to avoid small keff approximation
• Adds sweep_index to more EME data structures. Rearranges some of the other dims to be more consistent and user-friendly.
• Handling of sweep_index in smatrix_in_basis and field_in_basis
• Reorganizes some of the plotting functionality in sim_data.py to avoid code duplication. The main thing here was wanting a function plot_field_monitor_data that takes the monitor data directly as an argument instead of taking the monitor name.
• Adds normalize field to EMESimulation which specifies whether to normalize the port modes to unity flux (thereby normalizing the scattering matrix and expansion coefficients). True by default, but can be set to False in certain cases involving evanescent port modes.
• Other minor changes based on feedback: improved monitor storage size estimation, better handling of lists / np arrays, warning for large number of modes with constraint, convenience methods for port modes, cell-dependent EMELengthSweep

Todo:

• improve test coverage
• replace numpy operations with xarray operations in smatrix_in_basis and field_in_basis

Later:

• further improve code quality in smatrix_in_basis / field_in_basis

[caseyflex]

@tylerflex the test tests/test_plugins/test_adjoint.py::test_no_poynting is failing in this branch -- somehow the NotImplementedError in JaxSimulationData.get_poynting_vector is not being raised. Could you take a look? I tried to debug but I'm not too familiar with the adjoint tests.

[tylerflex]

@caseyflex due to some subtleties with how JaxSimulationData raises NotImplementedError when calling get_poynting_vector and now a lot of simulation data stuff calls _get_poynting_vector internally.

See the fix here:
https://github.com/flexcompute/tidy3d/pull/1636/files#r1619410128

[tylerflex]

by the way, should I just review once the to-do items are done and it's in it's final state?

[caseyflex]

You could wait on smatrix_in_basis and field_in_basis. Everything else should be pretty final already.

…

On Wed, May 29, 2024 at 9:07 PM Tyler Hughes ***@***.***> wrote: by the way, should I just review once the to-do items are done and it's in it's final state? — Reply to this email directly, view it on GitHub <#1636 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A3KLECNNFGN2OE22HPIKNF3ZEYYQZAVCNFSM6AAAAABGW7HU3KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMZYGE3TQNZVG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[tylerflex]

ok I guess ping me when it's done, thanks

[caseyflex]

ok I guess ping me when it's done, thanks

I think it is all ready for review now

[tylerflex]

Thanks @caseyflex . I think this looks good overall. I left some comments.

In general it seems there are a ton of edge cases being handled here. While I'm sure it's correct, it was a bit difficult to follow the complexity in the code when reading through. More importantly, though, I am a little concerned about making sure the edge cases are tested properly to avoid unexpected issues in use. I think adding some unit tests for these various edge would go a long way towards the stability of the code.

The command I use to test coverage is:

pytest -rA tests --cov-report=html --cov=tidy3d && open htmlcov/index.html

must also pip install pytest-cov

Another general comment is that I would suggest trying to use xarray instead of to_numpy() and using axis manipulation. One can imagine in the future if the order of some of the data dimensions gets changed, or we add another dimension, this can quickly become unsustainable. So if there's any way to rewrite the array manipulation bits using the named axes of xarray, that would also really improve the stability and readability of the code.

Thanks a lot for these improvements to the EME solver and all of the hard work to get them out!

[momchil-flex]

When I run through the web, it seems that the task monitoring stays stuck at "starting up solver" until it eventually goes to "success". This may not be something introduced by this PR but worth noting to fix.

[momchil-flex]

So currently EMESimulation supports both freqs and a sweep_spec that can be a frequency sweep, right? Providing multiple freqs will compute the modes exactly, while providing a frequency sweep spec will use the mode computation in the reduced basis? What happens if both are provided?

[caseyflex]

So currently EMESimulation supports both freqs and a sweep_spec that can be a frequency sweep, right? Providing multiple freqs will compute the modes exactly, while providing a frequency sweep spec will use the mode computation in the reduced basis? What happens if both are provided?

Yeah, that's right -- I should clarify in the docstrings. When both are provided, it kind of works as expected -- performs exact mode solves at all the freqs, and then applies the freq_scale_factors to each exact solution. Although this would definitely be an obscure use case.

[yaugenst-flex]

@caseyflex fyi, autograd tests are failing because of #1734 and will pass once #1736 is merged

[tylerflex]

As far as I'm concerned, this is good to go assuming whatever comments I had earlier are either resolved or marked somewhere for revisiting whenever possible. Thanks @caseyflex