Revamped add fields::dft_fields_norm2

[smartalecH]

Fixes #1711

Similar to #1718, except it does an L2 norm over the already calculated DFT fields, rather than manually recomputing (a modified version of) them. This means the user doesn't have to run this as often (before you had to "sample" fast enough to prevent aliasing). Plus there are fewer FLOPs with this approach.

I'm sure there's a reason @stevengj didn't do things this way, just not sure what that may be.

It passed the adjoint solver test suite locally. I reran the first tutorial and saw similar relative errors in the gradients as the old method.

We should probably do some profiling to see what convergence thresholds are really needed for "accurate" gradients.

[smartalecH]

(Also don't forget to squash before merging)

[oskooi]

Are you planning to add a unit test for this feature?

[stevengj]

Hi @smartalecH.

I considered this approach, but opted for the one in #1718 instead as being more conservative. The basic reason is that the norm of the fields used to update the DFTs (my approach) gives you an upper bound on the change in the norm of the DTFTs (your approach).

That is, if f is a vector of fields and f̂ is the corresponding vector of DTFTs, updated with f̂ += f exp(-iωt) Δt, then ‖Δf̂‖₂ ≤ ‖f‖₂ Δt for all frequencies ω. (Hence, ‖f‖₂ is proportional to an upper bound on the time derivative of the DTFT.) Moreover, this won't give a "false positive" for convergence if only the phase and not the amplitude of the DTFT is changing. (On the other hand, I suppose it could give a "false negative" if there are frequencies still propagating in the simulation that are only at frequencies you don't care about.)

I'm not sure what you mean by "aliasing" here, since the norm of the fields works as a bound for all frequencies at once. I don't think computational expense should be a concern, since it should be sufficient to sample the norm of the fields relatively infrequently, e.g. every few optical periods (as measured by fields::dft_maxfreq in #1718).

(That being said, either approach seems pretty reasonable.)

[smartalecH]

The basic reason is that the norm of the fields used to update the DFTs (my approach) gives you an upper bound on the change in the norm of the DTFTs (your approach).

Got it. This makes much more sense, thanks!

On the other hand, I suppose it could give a "false negative" if there are frequencies still propagating in the simulation that are only at frequencies you don't care about.

I was seeing quite a few "false negatives" when using the method in #1718. In fact, I was unable to achieve any sort of convergence -- even with very "fast" sampling (e.g. sub optical period). I tried quite a few "sample rates" but still didn't see any sort of convergence with the adjoint tests. Perhaps there's a small bug (although it looks pretty good to me...)? It's possible that the really small frequencies are taking forever to decay due to issues with PML or other dispersion effects (whereas that isn't a problem with this PR's method, since it's only looking at user-specified frequencies).

I'll try your original method again and report the results below.

[smartalecH]

stop_when_dft_decayed now supports both methods for computing the decay rate (see the method flag). Turns out both methods now give very similar results with the adjoint solver test. The default is the more conservative method in #1718.

The decay rate is calculated every 1/sim.fields.dft_maxfreq() timesteps for both methods, which means it is called much more frequently than before. As a result, the decay tolerance needs to b <1e-10 for the adjoint tests to pass. This doesn't mean the simulation is running longer than before -- quite the opposite actually. Because it's updating so frequently, it can shut off as soon as that tolerance is hit. Before, the update was only called every 50 timesteps (a rather large default value).

The OptimizationProblem no longer uses the decay_dt or decay_fields parameters, but I left them in to not break (mostly my) existing code. I'm sure with future releases we can drop those parameters.

[stevengj]

In principle, we should set how often we check this to be max(1/(maxfreq*dt), decimation factors) so that we never halt without updating the DFTs first.

[stevengj]

Since they are performing similarly, maybe we should just do the less conservative approach (your initial approach), and just call it dft_norm2 or even just norm2.

[smartalecH]

In principle, we should set how often we check this to be max(1/(maxfreq*dt), decimation factors) so that we never halt without updating the DFTs first.

Fixed. I added a function, min_decimation(), that pulls the minimum decimation factor from all the DFT monitors (very similar to dft_maxfreq().

Since they are performing similarly, maybe we should just do the less conservative approach (your initial approach), and just call it dft_norm2 or even just norm2.

Fixed.

[smartalecH]

I readded the original adjoint solver test file, test_adjoint_solver.py. It seemed to have been deleted in #1749.

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 92.59259% with 2 lines in your changes missing coverage. Please review.

Project coverage is 74.38%. Comparing base (e2739e4) to head (4112444).
Report is 390 commits behind head on master.

Files with missing lines	Patch %	Lines
python/adjoint/optimization_problem.py	50.00%	2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1740      +/-   ##
==========================================
+ Coverage   74.37%   74.38%   +0.01%     
==========================================
  Files          13       13              
  Lines        4573     4552      -21     
==========================================
- Hits         3401     3386      -15     
+ Misses       1172     1166       -6     

Files with missing lines	Coverage Δ
python/simulation.py	76.79% <100.00%> (+0.23%)	⬆️
python/adjoint/optimization_problem.py	70.08% <50.00%> (-2.58%)	⬇️

[oskooi]

Probably better to just remove this line here and also from test_adjoint_solver.py where it is also used?

[smartalecH]

Fixed.

[smartalecH]

Should be ready to merge once all tests pass.

[smartalecH]

Finally passing tests and ready to merge