-
Notifications
You must be signed in to change notification settings - Fork 629
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 off-by-one filtering errors. #2016
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2016 +/- ##
==========================================
+ Coverage 73.15% 73.57% +0.41%
==========================================
Files 17 17
Lines 4917 4896 -21
==========================================
+ Hits 3597 3602 +5
+ Misses 1320 1294 -26
|
For consistency, we also need to update the following lines in meep/python/tests/test_material_grid.py Lines 17 to 18 in 34b62e9
meep/python/tests/test_material_grid.py Lines 91 to 94 in 34b62e9
And also: meep/python/tests/test_get_epsilon_grid.py Lines 15 to 16 in 34b62e9
|
Fixed, thanks. |
CI doesn't want to trigger for some reason... |
python/adjoint/filters.py
Outdated
def compute_mg_dims(Lx,Ly,resolution): | ||
'''Compute the material grid dimensions from | ||
the corresponding resolution, x-size, and y-size. | ||
The grid dimensions must be odd. |
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.
Why?
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.
If the number of "taps" is not odd, it's no longer a zero-phase filter.
And the way that we do the FFT filtering requires that the number of taps must equal the size of the input.
Should be ready to merge now. The implementation is agnostic to the number of filter taps. No need to "add 1" or check that's it's odd. |
LGTM. |
It seems that the 2d filters in In addition, as one of the steps for fixing the off-by-one error, this PR forces the size of each kernel to be an odd number along every direction, which is indicated by |
Fixes #2012.
Note this is just a patch. A future PR could really improve the performance of our filtering, as described in #2012.
For now, this PR ensures that the impulse response is always Type I (even with an odd number of taps) so that it is zero phase. It adds a new function,
compute_mg_dims
, to help with this.Also adds a test to check for "off-by-one" errors.