-
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
Anisotropic material gradient support #1801
Conversation
@ianwilliamson, I'm not quite sure what's going on with the Could you run the test on your end and look into it a bit? |
From a quick glance, it looks like this is updating the API of Line 9 in e8c2023
Are there any other APIs that are going to be changed as part of this? Separately, it may be nice to orient towards reusing common functions between |
Oof -- I didn't realize we diverged. We certainly need to refactor and use the same codebase for both APIs. In addition to the change in the field components, the fields themselves must be stored on the Yee grid ( Either way, this will require some significant refactoring, as each component has a different array size that must be stored and carefully processed. (we should add cylindrical support too) |
I think the setting in utils.py: Line 71 in 5c02ff2
is consistent with what's in meep/python/adjoint/optimization_problem.py Line 218 in 5c02ff2
Am I missing something? |
Ah you're right! Sorry for the confusion (I've been at this way too long...) |
No worries. I don't think this would be too bad to factor into |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1801 +/- ##
==========================================
- Coverage 73.92% 73.52% -0.41%
==========================================
Files 17 17
Lines 4924 4876 -48
==========================================
- Hits 3640 3585 -55
- Misses 1284 1291 +7
|
Should now be ready for review/merging. @ianwilliamson, I ended up refactoring the two APIs quite a bit, such that all of the major calls are within As described in #1811, the gradients are still incorrect for materials containing conductivities. Resolving that is best suited for another PR since the issue existed before this PR. |
python/adjoint/utils.py
Outdated
'''We need to correct for the rare cases that get_dft_array | ||
returns a singleton element for the forward and adjoint fields. | ||
This only occurs when we are in 2D and only working with a particular | ||
polarization (as the other fields are never stored). Our get_gradient |
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.
Perhaps we can be more explicit here and state that in 2D, the Ez polarization consists of a single scalar Ez field.
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.
Fixed.
src/meepgeom.cpp
Outdated
meep::volume v(r); | ||
LOOP_OVER_DIRECTIONS(dim, d){ | ||
v.set_direction_min(d, r.in_direction(d)-sd/2); | ||
v.set_direction_max(d, r.in_direction(d)+sd/2); |
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.
The smoothing diameter sd
is in units of the voxel dimension Δx (=1/resolution) rather than in absolute units. This means sd/2
should sd/(2*Δx)
. This is similar to how it is set up in grid_volume::dV
:
Line 628 in fc5ea4a
const double hinva = 0.5 * inva * diameter; |
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.
I was just copying what's performed during initialization:
meep/src/anisotropic_averaging.cpp
Line 204 in 3eabcef
const double smoothing_diameter = 1.0; // FIXME: make user-changable? |
Am I misunderstanding how this is being used?
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.
Ah I think I see. The code also uses gv.dv()
to do some scaling under the hood. Got it.
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.
Fixed.
meep::vec eps2 = gv[ieps2]; | ||
cyl_scale = (gv.dim == meep::Dcyl) ? eps2.r() : 1; | ||
prod = 0.5*adj*get_material_gradient(eps2, adjoint_c, forward_c, fwd_avg, frequencies[f_i], geps, 1e-6); | ||
material_grids_addgradient_point(v+ng*f_i, vec_to_vector3(eps2), scalegrad*prod.real()*cyl_scale, geps); |
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.
There is redundant code here that can be separated into a function and called twice with different arguments (fwd_p, fwd_pf, eps1)
and (fwd_pa, fwd_paf, eps2)
.
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.
I thought so too... but everything worth refactoring is already refactored in get_material_gradient
and material_grids_addgradient_point
.
Pulling it out further just made it more unreadable (and requires a lot more parameters to pass than just those 3).
prod
could technically be placed within material_grids_addgradient_point
. cyl_scale
is repeated, but again, refactoring actually makes it harder to read.
self.d_E[nb][ic][f, :, :, :] = atleast_3d( | ||
self.sim.get_dft_array(dgm, c, f)) | ||
# Store forward fields for each set of design variables in array | ||
self.d_E = utils.gather_design_region_fields(self.sim,self.design_region_monitors,self.frequencies) |
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.
This is the D field now, correct? Perhaps this var should be renamed?
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.
Fixed.
|
||
if self.sim.is_cylindrical or self.sim.dimensions == mp.CYLINDRICAL: | ||
# Store adjoint fields for each design set of design variables | ||
self.a_E.append(utils.gather_design_region_fields(self.sim,self.design_region_monitors,self.frequencies)) |
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.
This is also the D field? Rename the var?
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.
Fixed.
python/adjoint/utils.py
Outdated
s = sim.get_array_slice_dimensions(c, vol=self.volume)[0] | ||
if (onp.squeeze(fields_a[ci][0,...]).size == 1): | ||
fields_a[ci] = onp.zeros(onp.insert(s,0,num_freqs)) | ||
fields_f[ci] = onp.zeros(onp.insert(s,0,num_freqs)) |
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.
It would be more readable to do onp.zeros((num_freqs,) + s)
. Also, would help to use a more descriptive name than s
... maybe spatial_shape
or something similar?
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.
maybe spatial_shape or something similar?
Good idea
onp.zeros((num_freqs,) + s)
unfortunately s
is a numpy array, and can't be joined like python lists. This seems pretty standard when prepending to numpy arrays, from what I've seen (there are hundreds of possibilities of course...)
python/adjoint/utils.py
Outdated
shapes = [] | ||
com = _compute_components(sim) | ||
scalegrad = 1 | ||
for ci, c in enumerate(com): |
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.
I suggest for component_idx, component
for better readability
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.
Fixed.
num_freqs = onp.array(frequencies).size | ||
shapes = [] | ||
com = _compute_components(sim) | ||
scalegrad = 1 |
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.
Can you add a comment as to why scalegrad
is needed? If it's hardcoded to 1 here, can it just be removed?
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.
Fixed.
python/adjoint/utils.py
Outdated
proper shape as the design_region. | ||
''' | ||
s = sim.get_array_slice_dimensions(c, vol=self.volume)[0] | ||
if (onp.squeeze(fields_a[ci][0,...]).size == 1): |
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 do you need the squeeze? Can't you just check the size?
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.
Fixed.
python/adjoint/utils.py
Outdated
def get_gradient(self, sim, fields_a, fields_f, frequencies): | ||
num_freqs = onp.array(frequencies).size | ||
shapes = [] | ||
com = _compute_components(sim) |
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.
Maybe just move the call to _compute_components()
inside enumerate(...)
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.
Fixed.
* init * get everything compiled and default passing tests * new start * anisotropic gradients are working * fix multifrequency bug and other cleanup * add support for cyl coordindates * add rotated sapphire to adjoint solver test * get tests passing locally * enable dispersive material support * some minor cleanup * clean up comments and readability issues and fix scaling of smoothing diameter Co-authored-by: Alec Hammond <[email protected]>
Closes #1794
This needs to work before #1780 (since subpixel smoothing induces anisotropic tensor elements).
Essentially, this PR properly recombines the forward and adjoint fields when calculating the gradients, regardless of what the material looks like (e.g. with or without off-diagonal elements). To do so, we store the
D
fields rather than theE
fields, which means we look atchi1inv
rather thanchi1
.The process is straightforward:
This is the proper interpolation/restriction scheme, as this is the gradient of what's actually implemented in the code.
As a nice side effect, this makes the gradients significantly more accurate (a few orders of magnitude when comparing relative errors for some initial tests).
TODO:
conductivemedia (I turned this off in order to simplify the code while debugging)NOTE: conductive media will require a separate PR (see #1811)