KLayout / klayout

KLayout Main Sources
http://www.klayout.org
GNU General Public License v3.0
782 stars 200 forks source link

Klayout PyCell integration #1641

Closed ThomasZecha closed 6 months ago

ThomasZecha commented 7 months ago

-added tl::optional as derivate of std::optional for c++17 and above, reduced implementation otherwise -fixed missing include for c++17 and above -added range constraints for PCell parameter

ThomasZecha commented 7 months ago

Thanks for the critical review.

I see a range constraint on the same conceptual level as choice constraint:

  1. limiting the possible values of a parameter
  2. are intuitively recognizable as such by the user
  3. are difficult for the user to operate incorrectly (Provide instant feedback if)

So why implement a range differently to a choice? Of course a range could be implemented by coerce_parameters but in a much less fashion regarding 2. and 3.

By the way: coerce_parameters delivers only a list of the parameter values, but not corresponding names. This could make an implementation quite unwieldy. Why not using a map?

klayoutmatthias commented 7 months ago

The choices are actually meant for mode parameters (strings), not for numerical values. Also, I see that many constraints are more complex, like a maximum area, which limits the product of two parameters. Such constraints are pretty common and cannot be captured by a simple range.

"coerce_parameters" also plays a different role: as the name implies it provides some normalization and some warranties to the layout production code. "coerce_parameters" is also called under other circumstances, specifically right before layout production happens. So even if the parameters are modified by script, the validity of the parameter set is established before "produce" is called.

If you rely on the limits implied by the range, it is not enough to capture the edit case. However, if you see the ranges as visual aids only, capturing them during edits is good enough, but not warranty is implied. This is important to note for the authors of the production function so their code can be aware of that case and raise an exception for example. In that context, getters for the range limit are very important, as they would allow friendly PCell instantiation code to check the limits before setting a parameter value.

Side note about the return value of "coerce_parameters": parameters are always lists internally, corresponding to the declaration order. This is for the internal interfaces and I cannot change that without breaking low-level API compatibility. For PCell authors there is the "PCellDeclarationHelper" wrapper which provides a more intuitive access layer.

There are some further comments I should make:

I am mentioning the "change of limits" case as I came across that problem a number of times in Cadence world. This is a real issue and not a hypothetical one.

I also need to say - and maybe it becomes clearer now - that although the feature may look trivial, it is not. The current code is the result of many iterations with users and constraint management was one of the key issues.

I am sorry for the many topics and modification requests, but I could have told you that in advance if I had a chance to.

Matthias