gdsfactory / gdsfactory

python library to design chips (Photonics, Analog, Quantum, MEMs, ...), objects for 3D printing or PCBs.
https://gdsfactory.github.io/gdsfactory/
MIT License
495 stars 208 forks source link

Incorrect rectangle size due to `snap_to_grid_2x` #3022

Open Daasein opened 1 month ago

Daasein commented 1 month ago

Describe the bug When creating rectangle at certain sizes, the true size of rectangle is in correct

To Reproduce If you create a rectangle with a decimal point of .785 in one dimension, the actual size of the rectangle will be 1 nm smaller.

import gdsfactory as gf
c = gf.components.rectangle(size=(3.785,2),layer=(1,0))
c.bbox_np()
# c.bbox_np()=array([[1.000e-03, 0.000e+00],
#                   [3.785e+00, 2.000e+00]])

image image

Expected behavior Because the decimal precision of the passed size is up to the third place and is not smaller than the size of dbu, the size of the rectangle should be exactly equal to size.

Reason image In gf.components.compass.py, snap_to_grid2x is used. When size is passed as (3.785,2) , it will be reduced to (3.784, 2).

Suggested fix Use snap_to_grid rather than 'snap_to_grid2x', and define the points in gf.components.compass.py as:

points = [
        [0,0],
        [dx,0],
        [dx,dy],
        [0,dy],
    ]
sebastian-goeldi commented 1 month ago

This is a snapping problem with ports. If you make the rectangle not 2xdbu sized, your port is offgrid. Since these rectangles have ports, this is not an option.

If you purely want a rectangle you can add it as follows:

c.shapes(LAYER.MYLAYER).insert(gf.kdb.DBox(left, bottom, right, top)) # where left, bottom, right, top are coordinates
Daasein commented 1 month ago

Hi @sebastian-goeldi ,

I fully understand the snapping problem. However, don't you think it is too complicated for a user to use the code you mentioned just to add a simple rectangle of the correct size?

kwonka commented 1 month ago

Noticed the same issue. It makes sense for centered rectangles to snap to "even" widths and heights if symmetry is the priority, but there is no reason that the same restriction should apply to uncentered rectangles. It's not the biggest deal, but it would be nice if these cases could be handled differently.

Edit: Nevermind, I realized there can be ports on the rectangle in this version. Even a no port or half port workaround seems convoluted and confusing to the user for no real benefit.

Quickly threw this together if anyone wants it, but as I said it's too convoluted for the generic PDK

@gf.cell
def rectangle(size=(4.0,2.0), layer="WG", centered=False, port_type='electrical', port_orientations=None):
    if centered==False:
        c = gf.Component()
        dx = size[0]
        dy = size[1]
        if port_orientations is not None:
            if (0 in port_orientations) or (180 in port_orientations):
                dy = gf.snap.snap_to_grid2x(dy)
            if (90 in port_orientations) or (270 in port_orientations):
                dx = gf.snap.snap_to_grid2x(dx)
            if 0 in port_orientations:
                c.add_port(name="e3", center=(dx, dy/2), width=dy, orientation=0, layer=layer, port_type=port_type)
            if 90 in port_orientations:
                c.add_port(name="e2", center=(dy/2, dx), width=dx, orientation=90, layer=layer, port_type=port_type)
            if 180 in port_orientations:
                c.add_port(name="e1", center=(0, dy/2), width=dy, orientation=180, layer=layer, port_type=port_type)
            if 270 in port_orientations:
                c.add_port(name="e4", center=(dy/2, 0), width=dx, orientation=270, layer=layer, port_type=port_type)
        c.add_polygon([[0,0],[0,dy],[dx,dy],[dx,0]], layer=layer)
        c.auto_rename_ports()
    else:
        c = gf.components.rectangle(size,layer,centered,port_type,port_orientations)
    return c
sebastian-goeldi commented 1 month ago

I don't think so, no. To be a fair comparison let's look at the full code:

compass

# define w,h for size
compass = c << gf.compass(size=(3,5), layer=LAYER.WG)
compass.move(x,y)

pure klayout:

b = gf.kdb.DBox(3,5)
b.move(x,y)
c.insert(LAYER.WG).insert(b)

I would say the difference is negligible, especially considering that you don't want a box with ports (otherwise it's simply impossible to have not 2*dbu multiple sizes).

joamatab commented 1 month ago

the snap to grid 2x is happening to avoid off-grid ports