Skip to content
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 segmentation fault in material_grids_addgradient of adjoint solver #1574

Merged
merged 1 commit into from
May 19, 2021

Conversation

oskooi
Copy link
Collaborator

@oskooi oskooi commented May 18, 2021

Fixes a segmentation fault in the function material_grids_addgradient_point of src/meepgeom.cpp in the following lines:

meep/src/meepgeom.cpp

Lines 2600 to 2603 in 8e2b0a0

// no object tree -- the whole domain is the material grid
if (!tp && is_material_grid(default_material)) {
vector3 pb = to_geom_box_coords(p, &tp->objects[oi]);
vector3 sz = mg->grid_size;

The problem is that tp (a pointer to a geometry tree) is a NULL pointer when entering this if-statement block which causes a segmentation fault when &tp->objects[oi] is invoked.

@oskooi oskooi added the bug label May 18, 2021
@oskooi oskooi requested a review from smartalecH May 18, 2021 23:04
Copy link
Collaborator

@smartalecH smartalecH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine after initial inspection -- would be good to add a simple test before merging.

@@ -2599,16 +2599,16 @@ void material_grids_addgradient_point(double *v, std::complex<double> fields_a,
}
// no object tree -- the whole domain is the material grid
if (!tp && is_material_grid(default_material)) {
vector3 pb = to_geom_box_coords(p, &tp->objects[oi]);
map_lattice_coordinates(p.x,p.y,p.z);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've never used this before, but I assume it's the same as above but assumes a box the size of the simulation cell (the lattice in this case)?

Maybe make sure there's no weird issues with the boundaries (when I read lattice I assume periodic boundaries, which obviously isn't always the case) just make sure the last edge in each dimension is mapped to "1" and not back to "0" (or vice-versa).

(Ideally the two routines use the same mapping logic and the only difference is the default geometry in this case)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function map_lattice_coordinates maps elements of the vector3 parameter p into the range [0,1]:

meep/src/meepgeom.cpp

Lines 374 to 381 in 8e2b0a0

void map_lattice_coordinates(double &px, double &py, double &pz) {
px = geometry_lattice.size.x == 0 ? 0
: 0.5 + (px - geometry_center.x) / geometry_lattice.size.x;
py = geometry_lattice.size.y == 0 ? 0
: 0.5 + (py - geometry_center.y) / geometry_lattice.size.y;
pz = geometry_lattice.size.z == 0 ? 0
: 0.5 + (pz - geometry_center.z) / geometry_lattice.size.z;
}

I think you were the one that set up the mapping this way. As long as p is defined to be within geometry_lattice, it should map points along the boundaries correctly.

@smartalecH
Copy link
Collaborator

Note that this fix is only for the case where the MaterialGrid is passed as a default material, which has never been rigorously tested (obviously since a pointer was being dereferenced when it was already determined to be NULL)

This seg fault is not associated with normal use with MaterialGrids inside of Block geometry objects (not clear from above synopsis).

@oskooi
Copy link
Collaborator Author

oskooi commented May 18, 2021

#1539 added a new unit test for MaterialGrid as the default material but only as part of a "forward" simulation (i.e., no gradient calculation). We do not yet have a similar test involving the adjoint solver which is why this bug has gone undetected.

@ianwilliamson
Copy link
Contributor

This seg fault is not associated with normal use with MaterialGrids inside of Block geometry objects (not clear from above synopsis).

Just wanted to jump in and say that I have observed segfaults from this function under the normal use that you describe (MaterialGrid in an mp.Block). I agree that what you're saying should be the case, but since the function doesn't have a unit test it's hard to say for sure what's actually happening here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants