New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Klayout PyCell integration #1641

Closed

ThomasZecha wants to merge 0 commits into KLayout:master from ThomasZecha:master

ThomasZecha commented Mar 6, 2024

-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

klayoutmatthias reviewed

View reviewed changes

Collaborator

klayoutmatthias left a comment

Thanks for the patch, but I have a general question - apart from a number of style issues:

The original hook for the implementation of such a constraint management is the "coerce_parameters" callback. Is there a reason why this feature cannot be used for a specific purpose?

The callback is somewhat more generic and allows more detailed decisions - e.g. parameter ranges based on other parameters or computed conditions like area constraints from a w and l parameter.

There is also the callback feature which can be used for that purpose.

Why I third concept?

src/db/db/dbPCellDeclaration.h Outdated

@@ @@ -30,6 +30,7 @@ @@
               #include "dbLayout.h"
               #include "tlVariant.h"
               #include "tlObject.h"
+              #include "tlOptional.h"

Collaborator

klayoutmatthias Mar 6, 2024

I don't necessarily see that this is needed for that particular case. low = high = nil could be used to indicate there is not range.

Author

ThomasZecha Mar 8, 2024

That's a general style question: Use a specific value of item('s) to decide whether it is used or not, or wrap it in an optional? I prefer using optional because of it's speaking name and readability increasing concept: I'm optional.

src/db/db/dbPCellDeclaration.h Outdated

@@ @@ -69,7 +70,7 @@ class DB_PUBLIC PCellParameterDeclaration @@
                  *  @brief The default constructor
                  */
                 PCellParameterDeclaration ()
-                  : m_hidden (false), m_readonly (false), m_type (t_none)
+                  : m_hidden (false), m_readonly (false), m_type (t_none), m_range()

Collaborator

klayoutmatthias Mar 6, 2024

Please follow style (blank between bracket and name). In general, I don't explicitly call default ctors.

Author

ThomasZecha Mar 8, 2024

Will be adjusted.

src/db/db/dbPCellDeclaration.h Outdated

+                enum Action {
+                  t_Reject = 1,   //  reject the parameter
+                  t_Accept,       //  accept the parameter
+                  t_Use_Default   //  use a default parameter (currently not supported)

Collaborator

klayoutmatthias Mar 6, 2024

Why having an option that is not supported?

Author

ThomasZecha Mar 8, 2024

Will be adjusted.

src/db/db/dbPCellDeclaration.h Outdated

+                 */
+                enum Action {
+                  t_Reject = 1,   //  reject the parameter
+                  t_Accept,       //  accept the parameter

Collaborator

klayoutmatthias Mar 6, 2024

What is the reasoning for a range that does not limit the values?

Author

ThomasZecha Mar 8, 2024

Coming from PyCell-API, e.g. for test-reason during development.

src/db/db/dbPCellDeclaration.h Outdated

+                  _Range() :
+                    m_low(),
+                    m_high(),
+                    m_resolution(),

Collaborator

klayoutmatthias Mar 6, 2024

I don't see the resolution being used.

Author

ThomasZecha Mar 8, 2024

Will be removed.

src/db/db/gsiDeclDbLibrary.cc Outdated

                   "This method will add the given value with the given description to the list of\n"
                   "choices. If choices are defined, KLayout will show a drop-down box instead of an\n"
                   "entry field in the parameter user interface.\n"
+                  "If a range is already set for this parameter the choice will not be added and a warning message is showed.\n"

Collaborator

klayoutmatthias Mar 6, 2024

Please do not implement it that way (see above)

Author

ThomasZecha Mar 8, 2024

Will be adjusted.

src/db/db/gsiDeclDbLibrary.cc Outdated

                   "This method will add the given value with the given description to the list of\n"
                   "choices. If choices are defined, KLayout will show a drop-down box instead of an\n"
                   "entry field in the parameter user interface.\n"
+                  "If a range is already set for this parameter the choice will not be added and a warning message is showed.\n"
+                ) +
+                gsi::method_ext ("set_range", &set_range, gsi::arg ("low"), gsi::arg ("high"), gsi::arg ("resolution"), gsi::arg ("action"),

Collaborator

klayoutmatthias Mar 6, 2024

There is no corresponding getter.

Author

ThomasZecha Mar 8, 2024

Will be added.

src/db/db/gsiDeclDbLibrary.cc Outdated

+                  "@brief Set a range constraint\n"
+                  "This method will set a range constraint to the parameter with 'low' and 'high' as minimum and maximum value.\n"
+                  "This range constraint will only be set if the parameter-, the low- and the high-type are numeric.\n"
+                  "If a choice is already set for this parameter the range will not be set and a warning message is showed.\n"

Collaborator

klayoutmatthias Mar 6, 2024

Do not implement that this way.

Author

ThomasZecha Mar 8, 2024

Will be adjusted.

src/db/db/gsiDeclDbLibrary.cc Outdated

+                  "The optional parameter 'resolution' will give a desired resolution value (currently not used).\n"
+                  "The optional parameter 'action' determines the action to be invoked.\n"
+                  "This action can be one of three values: REJECT, ACCEPT, USE_DEFAULT. If this failure action\n"
+                  "parameter is not specified, then it will be REJECT by default.\n"

Collaborator

klayoutmatthias Mar 6, 2024

There is no default given.

Author

ThomasZecha Mar 8, 2024

Coming from PyCell API. Will be removed.

src/db/db/gsiDeclDbLibrary.cc Outdated

+                  "The optional parameter 'action' determines the action to be invoked.\n"
+                  "This action can be one of three values: REJECT, ACCEPT, USE_DEFAULT. If this failure action\n"
+                  "parameter is not specified, then it will be REJECT by default.\n"
+                  "If a range constraint is violated this parameter is marked wrong with violation hint in the\n"

Collaborator

klayoutmatthias Mar 6, 2024

Language: "If the entered value is not within the specified range, a warning will be shown in the user interface".

Author

ThomasZecha Mar 8, 2024

Will be adjusted.

Author

ThomasZecha commented Mar 8, 2024

Thanks for the critical review.

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

limiting the possible values of a parameter
are intuitively recognizable as such by the user
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?

Collaborator

klayoutmatthias commented Mar 8, 2024 •

edited

Loading

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:

Please do not add features that are "not supported yet". Unless you want to become permanent maintainer of the code yourself, everyone coming along such features will expect me to eventually implement it.
The range feature should be noted in the documentation too. The main documentation for PCell authors is embedded into src/db/db/built-in-macros/pcell_declaration_helper.lym and src/db/db/built-in-pymacros/pcell_declaration_helper.lym.
When you test your implementation, please try the following scenario: create a layout with a PCell and certain parameters and no range set. Save this layout. Then change the PCell implementation such that you introduce a range and the saved PCell violates the range. When you load the layout again, the implementation is supposed to indicate the current parameters as "outside the limits" when you check the PCell parameters in the UI. Unless you cancel the dialog, the implementation should not allow you to commit the original values. Specifically, this means that range checks are not only required on edits, but also on initialization of the UI. Please check, if your implementation does so.
Another related topic are the stored "recent" parameters of PCells. KLayout offers to use recently used PCell configurations on the "Recent" tab in the editor options. In the above scenario, it may happen that a PCell parameter set becomes invalid after changing the range and restarting the application. The application should behave reasonable in that case and refuse placing such a PCell.

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

ThomasZecha closed this

ThomasZecha force-pushed the master branch from eea4344 to ef53700 Compare

March 21, 2024 14:37

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

Labels

None yet