NanoComp / libctl

Guile-based library implementing flexible control files for scientific simulations
GNU General Public License v2.0
19 stars 22 forks source link

Add sidewall angle parameter #53

Closed danielwboyce closed 4 years ago

danielwboyce commented 4 years ago

Fixes NanoComp/meep#1067.

stevengj commented 4 years ago

Sounds like you still have to test intersect_line_with_prism and point_in_prism somehow.

stevengj commented 4 years ago

As a quick test, if the sidewall angle = 0, then it should match the results of the current code, which we think are correct. (This is also called "gold master" testing.)

danielwboyce commented 4 years ago

Taking a closer look at normal_to_prism I've noticed that if a test point is on the prism surface it doesn't return the zero vector, which is what I would expect. This is a behavior that, as far as I can tell, has been in place as long as normal_to_prism has existed. I propose we change normal_to_prism so that it returns the zero vector when minimum distance to the prism is 0, else it just returns nonsense results at points that are on an edge or corner. Alternatively, we could return a vector the norm of which is the minimum distance to the prism, instead a unit vector. Is there a particular reason why we've been return a unit vector instead of the vector to the nearest point on the prism, anyway?

danielwboyce commented 4 years ago

I've also noticed that point_in_prism has a bug where it's not finding points on the prism (including the version on the master branch currently).

danielwboyce commented 4 years ago

The above changes to intersect_line_with_segment and node_in_or_on_polygon seem to be causing small ridges to form in polygons with mostly orthogonal corners, see here:

comparison_orthogonal_16_1

comparison_orthogonal_32_3

(These polygons come from a tarball of test polygons that come from a paper comparing the effectiveness of point in polygon algorithms, discussed in NanoComp/meep#864 and #48. Hundreds of polygons were tested after these changes were made; only some, but not all, polygons with axis-parallel orthogonal corners exhibited this problem.)

Despite these changes making the point_in_prism checks pass, more changes will probably be have to be made to intersect_line_with_segment and node_in_or_on_polygon to fix these ridges.

stevengj commented 4 years ago

I wouldn't worry about discretization artifacts arising from rounding errors for points lying "exactly" on interfaces. (For example, if you displace the prism by 1e-8 in both x and y, I'm guessing that the artifacts will disappear.) These things won't matter once we do subpixel smoothing.

oskooi commented 4 years ago

Follow up from today's discussion: it might be useful to also verify that the (complicated) prism geometries can be reliably exported to a GDS file using gdspy. Should be straightforward but still important as a practical matter.

danielwboyce commented 4 years ago

With the most recent update I've got the test suite working for volume checking, point in prism checking, and normal to prism checking with all the test points for those functions passing. What remains for me to do before this can be merged is:

danielwboyce commented 4 years ago

With the most recent commit we have all the critical features for this merge in place:

Running make check a few thousand times has it failing only about 5.9% of the time. I'm working on diagnosing those issues in case we want better performance.

danielwboyce commented 4 years ago

Running make check a few thousand times has it failing only about 5.9% of the time. I'm working on diagnosing those issues in case we want better performance.

This actually seems to be a bug I had run into before. Each time make check runs it does 1,000 tests to find the intersection length between a line segment and a prism, checking against the intersection length found when the prism is constructed as a block. (This is unit test 3 in utils/test-prism.c.) When I investigated this before, I found that whenever make check fails it's due to one of the randomized intersection tests having a small floating point error (of about 1%, but the typical threshold is 1 part in a million). My judgement is that this is a small enough error that we're probably still fine to merge.

stevengj commented 4 years ago

Whoops, looks like this is breaking the Meep build because the typemap utils explicitly reference the prism->vertices list here.

We should undo the rename of vertices to vertices_bottom since that is breaking.

stevengj commented 4 years ago

vertices_bottom renamed in 7f31263c718779039c27152c45c339e1281cf398

stevengj commented 4 years ago

@danielwboyce, Meep now compiles again, but the mode_coeffs test is failing, and I'm guessing this may relate to the prism handling. Can you take a look?

thchr commented 4 years ago

I think this is breaking the MPB CI builds because variable declaration inside for loops is not C89 compatible. See https://travis-ci.org/NanoComp/mpb/jobs/649653306#L1075-L1087 (build failure in https://github.com/NanoComp/mpb/pull/112).

I can make a PR to fix this if there's agreement with the above.

stevengj commented 4 years ago

@thchr, a PR to fix the declarations to be C90-compliant would be welcome.

(Would also be good to exercise this in the tests.)