-
Notifications
You must be signed in to change notification settings - Fork 626
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
Fix FourierFields adjoint for D and B fields in isotropic media #2095
Conversation
It would be useful to add a unit test in this PR which should fail using the current master branch since this is a bug fix. |
However, the current version of this PR does not yield correct adjoint gradients if the monitor is placed inside the design region. Should we deal with this case? |
Lines 3003 to 3006 in 91d36ba
Actually.... I think we want the |
Looks fine, albeit missing a test. I agree that it would be good to add support for D fields in the design region, but this is nontrivial, and meanwhile this PR is a strict improvement over what we have now. |
0a35790
to
a816bc9
Compare
I changed an existing test in |
python/tests/test_adjoint_solver.py
Outdated
print("Ez2 -- adjoint solver: {}, traditional simulation: {}".format(adjsol_obj,Ez2_unperturbed)) | ||
self.assertClose(adjsol_obj,Ez2_unperturbed,epsilon=1e-6) | ||
print("Dz2 -- adjoint solver: {}, traditional simulation: {}".format(adjsol_obj,Dz2_unperturbed)) | ||
self.assertClose(adjsol_obj,Dz2_unperturbed,epsilon=1e-6) |
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.
Looks like this assert statement is failing for the single-precision build. You can try increasing the tolerance for single precision using e.g.:
self.assertClose(adjsol_obj,Dz2_unperturbed,epsilon=1e-4 if mp.is_single_precision() else 1e-6)
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.
In my place, this line does not cause error even with single precision. I guess the error message in my place is different from that given by the online check. But I cannot see the detailed error message in the online-check logs. Can I ask where I should find the detailed error message?
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.
To find the log of the failing CI run, click on the "Details" button below next to "CI / Test Python 3.10 with MPI (true)" or the "Actions" tab above (next to "Discussions" and "Projects"). From there, for the failing run locate the "Artifacts" section and then download the log file. In this case, this would be py3.10-tests-mpi-true.log which shows:
Ran 7 tests in 307.004s
FAILED (failures=1)
FAIL: test_complex_fields (__main__.TestAdjointSolver)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/runner/work/meep/meep/build/meep-1.24.0-beta/_build/sub/python/../../../python/tests/test_adjoint_solver.py", line 525, in test_complex_fields
self.assertClose(adj_scale,fd_grad,epsilon=tol)
File "/home/runner/work/meep/meep/build/meep-1.24.0-beta/python/tests/utils.py", line 28, in assertClose
self.assertLessEqual(diff_norm, epsilon * np.maximum(x_norm, y_norm), msg)
AssertionError: 0.001776412917294544 not less than or equal to 0.0005871758943843297 :
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! The tolerance for single precision is adjusted further.
This PR fixes #2090.
FourierFields
adjoint gradients for D and B fields in isotropic media now become consistent with finite-difference gradients.