Add to_gds to Simulation, Structure and Geometry (#883)

[lucas-flexcompute]

Structures with custom medium are intersected with their medium contours at a given permittivity threshold.

[tylerflex]

Thanks @lucas-flexcompute this looks great. I do have a general comment / question:

In the geometry base, we have a from_gds() classmethod, which loads the geometry from a user-supplied gds Cell (either gdspy or gdstk). For consistency, do you think it makes sense for this to_gds() to also accept a Cell object? I think this way we also never need to worry about importing gdstk or gdspy because the user specifies the cell and we just call whatever methods it has.

Just wondering if you think this is a better or worse alternative to the filename approach and why you chose that, thanks!

[lucas-flexcompute]

We'd still need to import the module to be able to create gdstk.Polygon or gdspy.Polygon, but your symmetry point makes sense. We could have the first argument be either a file name (str) or a Cell, in which case we just add the polygons to the given cell. I just don't think we should support only cell output because having a straight-to-file export function is very handy as well.

[lucas-flexcompute]

After looking a bit deeper into the cell/file output option: making the argument change the function behavior by type is usually a bad idea… Maybe we leave the to_gds functions as they are (misilar to to_json etc.) and expose _as_gds_polygon_list to the user under different names (to_gdstk and to_gdspy). The user can add them to any cells/libraries as they want.

[tylerflex]

Yea I agree, let's not have the same function do different things based on inputs.

One other option is to expose I guess 4 functions:
to_gdstk(gdstk.Cell)
to_gdspy(gdspy.Cell)
to_gds(Union[gdstk.Cell, gdspy.Cell]) # which does some switching to call one of the top 2, but is at least symmetric with from_gds().
to_gds_file()that calls to_gds() and then loads it into the provided file?

[lucas-flexcompute]

One other option is to expose I guess 4 functions: to_gdstk(gdstk.Cell) to_gdspy(gdspy.Cell) to_gds(Union[gdstk.Cell, gdspy.Cell]) # which does some switching to call one of the top 2, but is at least symmetric with from_gds(). to_gds_file()that calls to_gds() and then loads it into the provided file?

to_gdstk and to_gdspy don't need to get a cell if they just return the expected polygons — similar to to_shapely if we had something like that. Using a cell makes it harder for the user if they want to do anything else to the polygons, whereas if they just want to add them to a cell, cell.add(*structure.to_gdstk(x=x)) just works.

We'd still leave to_gds(cell) as the symmetric counterpart to from_gds. So they are options on different levels of abstraction both in their arguments and return values.

What do you think?

[tylerflex]

That sounds great to me. (I wasn't sure what to_gdstk / to_gdspy should return but what you describe sounds good.

[lucas-flexcompute]

@tylerflex and @tomflexcompute I've updated the interface according to the last discussion. There's some code duplication now that I find hard to avoid, because Geometry, Structure and Simulation have specific to_gdstk and to_gdspy versions, but the higher level to_gds and to_gds_file share some code.

One alternative would be to create global functions _to_gds and _to_gds_file that accept Geometry, Structure or Simulation to factor the common code. However, this function would depend on Simulation for type annotation and would be used in geometry.base, so it would create a circular dependency unless we leave it without annotation. It would also be generic in the other arguments because each object has a different signature for to_gdstk and to_gdspy:

def _to_gds(object, x, y, z, **kwargs):
    ...
    # remove unwanted kwargs
    object.to_gdstk(..., **kwargs) or object.to_gdspy(..., **kwargs)
    ...

Any other ideas or preferences?

[tylerflex]

I feel in this instance, some code duplication is probably preferable. I looked over the current implementation and I think it works as is.

Otherwise, we could have the generic versions accept Tidy3DBaseModel as argument _to_gds(object : Tidy3DBaseModel) and throw a warning if not hasattr(object, "to_gds"), not sure how much better that is.

What is your preference?

[lucas-flexcompute]

I think I prefer it the way it is right now. Adding an extra member to Tidy3DBaseModel seems like polluting the whole namespace of objects for (in this case) little gain. Let's wait to see if @tomflexcompute has other interface suggestions, though. A fresh pair of eyes might see something we're missing w.r.t. how the users would interact with these features.

[tomflexcompute]

Thanks @lucas-flexcompute . The interface looks good to me. Also works very well based on my limited testing.

So as far as my understanding goes, the conversion of tidy3d simulation or structure to gds is based on the permittivity distribution, which makes sense. One corner case I can think of now is when a user only makes half (or even less commonly, a quarter) of a structure and uses symmetry in the simulation (one example would be this). In this case, the exported gds file only contains half of the structure too, In my opinion the ideal behavior of to_gds from a simulation should automatically copy/flip the structure and make a full structure if symmetry is applied. Do you think so?

I tested it on this example) and the result gds file contains many fragments instead of one single geometry. Some manual cleaning can be done in KLayout for example to clean it up relatively easily so maybe that's not a huge concern.

One last comment I have is that the exported gds contains structures extended outside of the simulation domain. If td.inf is used to create certain geometries, the gds file will contain this huge structure and you really need to zoom in to see the actual device structure.

[lucas-flexcompute]

Thanks, @tomflexcompute! Yes, what you're proposing makes sense: to limit the gds export region using the simulation bounds and to apply the symmetry conditions. This is the new default behavior. I can add arguments to control that in the method call, but I'm afraid it already has too many parameters. Furthermore, the user can always export directly from Structure if they don't want the simulation bounds/symmetry expansion.

Regarding the fragments, that's the conventional way to represent geometries with too many vertices and holes in gds files. The user should be well aware of that if they work with gds files.

[tomflexcompute]

Thanks @lucas-flexcompute for the new implementation. It looks very good to me. Making the to_gds from Simulation consider symmetry and simulation boundary by default makes sense. Like you said, if an user doesn't want to include this, they can call to_gds from Structure.

[tylerflex]

@lucas-flexcompute if this is ready to merge, feel free to "rebase and merge" it. I wasn't sure if there were any last minute additions so will let you handle it, thanks.