Skip to content

Geometric object source#635

Closed
HomerReid wants to merge 13 commits intoNanoComp:masterfrom
homerredux:geometric_object_source
Closed

Geometric object source#635
HomerReid wants to merge 13 commits intoNanoComp:masterfrom
homerredux:geometric_object_source

Conversation

@HomerReid
Copy link
Contributor

Fixes #612. Adds a new optional obj argument to the python Source constructor, which may be used in place of the existing center/size parameters. If used together with those arguments, the source region is the intersection of the object with the volume.

Sample usage:

   source_cyl=mp.Cylinder(center=src_center, radius=radius)

   sources = [mp.Source(src=src_time, obj=source_cyl, component=src_cmpt)]

An obvious question is how one might go about testing and verifying this functionality, since there isn't any API support for visualizing or otherwise querying source distributions. I will address this point in a companion PR.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.07%) to 82.3% when pulling ea79a3680b05f8d46073ef7dbd84cc737b0efd71 on HomerReid:geometric_object_source into bc2330a on stevengj:master.

python/meep.i Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we should probably check if $input is an instance of a mp.GeometricObject. We need a function py_geometric_object similar to py_source_time_object, and then this typemap can be

{
  $1 = PyObject_IsInstance($input, py_geometric_object()) || $input == Py_None;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, that makes sense. I wasn't sure what to put in the body of this typemap.

src/meep.hpp Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need a new add_volume_source method here? Can't we just use the complex<double> A(const vec &) interface for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This implementation dates back to before the merge of meepgeom into meep, at which point libmeep had no knowledge of geometric_objects, so any implementation of this feature necessarily involved some sort of external function access. The feature could have been implemented using an amplitude function in python, but I was concerned that would be slow. Now that geometric_objects are available in libmeep, I'm sure you could implement the feature in a way that routed everything through the existing complex<double> A(const vec &) interface, perhaps with the object in question stored as a global variable, if you thought that would be a significant improvement over the above in terms of code efficiency, readability, or some other metric.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not a question of efficiency, but I'd rather not have two arguments (indicator and A) that are completely redundant in the underlying C++ code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Three of the four prototypes for add_volume_source actually don't take the A argument, so indicator is not redundant in those cases.

Even in the one remaining case, although it is true in principle that the geometric_object functionality could be routed through the existing A interface, I'm not seeing how this could be done gracefully. In this case the caller has provided an A function plus a separate geometric_object that don't know about each other, and we would have to write our own new A function that (1) tests the point for inclusion in the object, and if that passes (2) separately call the user's original A function to get the amplitude. Of course the A interface doesn't allow for passage of any user data, so both the geometric object and the caller's original A function would need to be passed to our wrapper A function via some other mechanism, probably global variables. I agree that it would be possible, but it feels sort of like bending over backwards to use the legacy A interface in ways for which it wasn't intended.

Plus, any way you slice it the prototype for add_volume_source is going to have to be modified to accept at least one new parameter for the geometric_object in one form or another, so it's not as if the legacy interface could simply be preserved in full pristine form anyway. My solution adds precisely one new parameter, almost no additional code, and zero global variables. Again, I agree that it would be possible to do what I think you have in mind, but it doesn't seem an obvious improvement over my solution---which has the further virtue of being already implemented and ready for users to start exploring.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Even if you have a new high-level interfact that takes a geometric_object, internally it can just construct an amplitude function and pass that to the lower-level add_volume_source routine. That way you don't need the whole meep::source_indicator thing, and low-level functions like src_vol_chunkloop etcetera don't need to be modified at all.

@oskooi
Copy link
Collaborator

oskooi commented Dec 24, 2018

Based on Meep's design philosophy of discretizing time and space to create an illusion of continuity, it would be nice for the geometric object sources to also: (1) perform some kind of averaging around object boundaries and (2) support zero-thickness objects in 3d. Adding this functionality does not require anything new since it can be derived from existing averaging routines for subpixel smoothing (i.e., analytically computing the volume filling fraction of boundary pixels) and sources (i.e., "restricting" the current source to the Yee grid using weights as shown in Fig. 4 of the Meep paper).

@HomerReid HomerReid mentioned this pull request Dec 31, 2018
@stevengj
Copy link
Collaborator

For Ardavan's comment, the amplitude-function would call box_overlap_with_object instead of point_in_object to return a value between 0 and 1.

@stevengj
Copy link
Collaborator

rebased

@stevengj
Copy link
Collaborator

I just merged a PR #662 that reformats everything with clang-format, which creates lots of conflicts.

To rebase this PR onto master without having to deal manually with these formatting conflicts, follow the exact procedure given in the instructions of #662.

@stevengj stevengj force-pushed the geometric_object_source branch from cd08730 to 0e88244 Compare January 11, 2019 14:13
@stevengj
Copy link
Collaborator

Rebased.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants