Added support for created/modified, layerdat tuples and additional GdsIm...

tom-varga commented 9 years ago

Some gds files contain extra records that are unused by following records. For example a TEXTYPE before a PATH record. It appears that cad tools correctly ignore these records. However, gdsCAD's use of the _kwargs mechanism results in these unexpected records being passed to an object that doesn't expect it. Hence, _ignore was added to a few of the class init functions.

Support was added to _anchor for a consistent 2-character '{tcb}{lcr}' anchor naming convention.

Added x_reflection to Text str

Added support for the BGNLIB and BGNSTR created and modified fields. The Layout and Cell classes were enhanced to support the setting and accessing of these properties.

Modern gds files use datatypes for most layers. Hence get_layer is useless especially if there isn't a get_datatype for each layer. Hence get_layerdats and objects_by_layerdat accessors were added that return or use (layer, datatype) tuples.

Additional verbosity was added for GdsImport when verbose==2. Details of every record where possible are also printed.

hohlraum commented 9 years ago

Wow, you've been busy Tom. That's an impressive number of new additions.

I'm wary of using **ignore to collect unsupported records. There's already no typechecking on arguments (e.g. Cell_Reference('cell') doesn't raise an error), so I don't like the idea of, for instance, mistyped keyword arguments just disappearing. My concern is not when this is used with the import function, I worry that fixing the import problem will lead to more confusion in other situations.

Better, perhaps, to clean the arguments in the _create_x wrapper. For instance, change the path creation wrapper to:

def _create_path(xy, width=1.0, layer=None, datatype=None, pathtype=0, **ignore):
    points = xy.reshape((xy.size // 2, 2))
    return Path(points, width, layer, datatype, pathtype)

We could maybe add a warning too listing the ignored arguments.

The two-character anchor codes are great.

x_reflection for Text is good.

Creation/modification time. Three issues here:

I would prefer these to be attributes (i.e. with @property decorator) rather than methods. I know the code's not totally consistent, but simple information about objects is being added as an attribute rather than using getters and setters. One consequence is that there cannot be a string option, but this shouldn't be too bad since str(t) returns a formatted string for a datetime object.
Since we now have real creation and modification dates these should be used in to_gds() rather than the placeholder now.
Perhaps include a way of updating the modified property through update_modified() method (only allow updating to now()) , or Cell.modified=some_datetime (allow updating to any time). Which do you think is better?

get_layerdats / objects_by_datatype

I'm worried about the asymmetry between get_layers and get_layerdats. The former returns the layers in the current cell and all children. The latter returns the layers (and datatypes) of only the current cell. Is there a good reason for one to behave one way, and not the other? Can/should the two methods be combined into one? I'll have to think about this.

If nothing else I should update the docstring for get_layers so that it is explicit about returning its children's layer info as well.

Verbosity is good.

Valuable additions all. Let me know what you think about these comments.

tom-varga commented 9 years ago

Let me start by saying that I'm not the most experienced python programmer yet, so I'm more than happy to learn how to do things more pythonic.

For the unsupported records issue, I had originally thought about using a wrapper to check arguments which would then simply ignore unexpected arguments. I think your suggestion is fine. However, since I'm finding that some tools create gds files with extra records that should be ignored, I'd prefer not to see thousands of warnings when reading in large gds files. Maybe the default should still be to quietly ignore the extra records as cleanly and safely as possible.

For the creation time questions, I agree with 1. I tried to do it that way, but obviously don't understand the concept well enough yet. For 2. I realized after the fact that to_gds() should be updated. So, I agree. Point 3 sounds good. I don't see a reason to update to any time other than now but it could be useful to somebody.

About get_layers and get_layerdats, my needs are with the specific cell in question and not its children. That being said, maybe both get_layers and get_layerdats could have an additional argument such as depth so that by default it would only return layers or layerdats to the requested depth. I don't care whether the default is like the current get_layer or get_layerdats behavior so long as I can specify what I want.

-Tom

On Thu, Feb 26, 2015 at 4:25 AM, hohlraum notifications@github.com wrote:

Wow, you've been busy Tom. That's an impressive number of new additions.

I'm wary of using **ignore to collect unsupported records. There's already no typechecking on arguments (e.g. Cell_Reference('cell') doesn't raise an error), so I don't like the idea of, for instance, mistyped keyword arguments just disappearing. My concern is not when this is used with the import function, I worry that fixing the import problem will lead to more confusion in other situations.

Better, perhaps, to clean the arguments in the _create_x wrapper. For instance, change the path creation wrapper to:

def _create_path(xy, width=1.0, layer=None, datatype=None, pathtype=0, **ignore): points = xy.reshape((xy.size // 2, 2)) return Path(points, width, layer, datatype, pathtype)

We could maybe add a warning too listing the ignored arguments.

The two-character anchor codes are great.

x_reflection for Text is good.

Creation/modification time. Three issues here:

I would prefer these to be attributes (i.e. with @property decorator) rather than methods. I know the code's not totally consistent, but simple information about objects is being added as an attribute rather than using getters and setters. One consequence is that there cannot be a string option, but this shouldn't be too bad since str(t) returns a formatted string for a datetime object.

Since we now have real creation and modification dates these should be used in to_gds() rather than the placeholder now.

Perhaps include a way of updating the modified property through update_modified() method (only allow updating to now()) , or Cell.modified=some_datetime (allow updating to any time). Which do you think is better?

get_layerdats / objects_by_datatype

I'm worried about the asymmetry between get_layers and get_layerdats. The former returns the layers in the current cell and all children. The latter returns the layers (and datatypes) of only the current cell. Is there a good reason for one to behave one way, and not the other? Can/should the two methods be combined into one? I'll have to think about this.

If nothing else I should update the docstring for get_layers so that it

is explicit about returning its children's layer info as well.

Verbosity is good.

Valuable additions all. Let me know what you think about these comments.

— Reply to this email directly or view it on GitHub https://github.com/hohlraum/gdsCAD/pull/24#issuecomment-76146496.

hohlraum commented 9 years ago

I've resolved all of the issues in this pull request, except what to do with get_layers/get_layerdats. You can find them in the branch pr24.

hohlraum commented 9 years ago

OK, I've commited a modified version of this pull request. A warning that the call signature for get_layers/get_layerdats may change once I make up my mind.

hohlraum / gdsCAD