Closed PietroCampana closed 1 month ago
Thank you for your pull request and welcome to our community.
We require contributors to sign our Contributor License Agreement (CLA), and it seems that the following contributors have not yet signed it: @PietroCampana.
In order for us to review and merge your code, please sign the CLA at meetiqm.com.
Once you have signed the CLA, write a comment here and we'll verify. Please note that it may take some time for us to verify it.
I have now signed the CLA.
Thank you for your pull request and welcome to our community.
We require contributors to sign our Contributor License Agreement (CLA), and it seems that the following contributors have not yet signed it: @PietroCampana.
In order for us to review and merge your code, please sign the CLA at meetiqm.com.
Once you have signed the CLA, write a comment here and we'll verify. Please note that it may take some time for us to verify it.
I have added the remove_kqc_pcells
function to separate the deletion of PCells instances from the collection of their data. This requires to iterate over all the views and PCells one more time, but if errors occur when reloading the libraries the current view is kept instead of trying to redraw the PCells with possibly uninitialized libraries.
Thanks for the pull request. Unfortunately there is a hidden bug in this approach, that can be replicated easily:
_build_island2
return pya.Region()
Reload Libraries
Move
tool, click the element once, and click another time to some other place in the layout to move it out of the way. You'll see there were actually two elements stacked on top of each other, one with pre-change geometry and other post-change.I believe what is happening is that after load_libraries
is called, the element placed on layout stops being a subclass of KQC Element
. Hence when we attempt to clean it up with remove_kqc_pcells
, it does not register as KQC Element
and is left alone. While also restore_pcells_to_views
will place an updated copy of it on top of it.
While it's probably good that inst.delete
is not called during extract_pcell_data_from_views
, I think they still should be collected into some list, so that if load_libraries
does not cause errors, the list is iterated to remove old version elements.
@cla-bot check
The cla-bot has been summoned, and re-checked this pull request!
Another thing I recommend to test is to induce a syntax error to test that the try
block for reload_libraries
picks up the error correctly. If for example you modify the start of _build_island2
function to be:
def _build_island2(self, squid_height, taper_height):
if True:
island2_top = self.squid_offset - squid_height / 2
island2_polygon = pya.DPolygon(
[
...
the if True:
is not indented correctly, which will cause your error to show. From my tests what happens is that I no longer become able to choose "Qubit Library" since KQC code is broken, but when I "fix" the code error and run Reload Libraries
again, the "Qubit Library" panel becomes available again. It would be nice if the qubit I placed on the layout becomes modifiable again after the code errors are fixed, this is something that doesn't happen in this implementation.
Another quirk is that the newline after if True:
is quite important for some reason. If this is removed, running Reload Libraries
causes KLayout to crash without any messages. Restarting KLayout and looking at File > Log Viewer
will print some unrelated errors. I think for the purposes of the hackathon we can leave such weird bugs unaddressed if they are hard to debug.
I have written a partial solution addressing both points. To solve the problem of duplicated instances, they are now collected at the same time of the view data in the extract_pcell_data_from_view
function and deleted later. In this way every PCell that is drawn again gets deleted, leaving no duplicates.
Making the PCell modifiable again after fixing the libraries in case of any error seems to be fairly more complex, but I found a possible simple solution by moving the call to _get_all_pcell_classes
at the beginning of load_libraries
, before the libraries are deleted. Since the modules are actually reloaded in _get_all_pcell_classes
, in this way syntax errors in the components definitions stop the function before deleting the libraries, and the KQCircuits PCells remain Element
instances, which will be tracked until the error is fixed. However, in the case of errors appearing when rebuilding the libraries, the original ones will already have been deleted, losing the Element
instances. At the moment the only way to keep track of the cells even when libraries are completely deleted that I can think about is to save the original PCell data collected in extract_pcell_data_from_view
into a temporary file, for example by pickling them.
Regarding the last point, I couldn't manage to reproduce the quirk due to the newline after is True
. On my installations (Arch linux and Ubuntu) it either generates additional error messages in the log or doesn't change behavior at all, but Klayout doesn't crash. I will send more detailed specs if you want to investigate this further.
Thank you for your follow-up. Right now this seems to work fantastic! If I place an element, induce some code errors, reload libraries, it gives me errors and then lets me modify parameters for the element based on the previous version of the code. I only tested on Windows so far, does it perform differently on Ubuntu/Arch? Because as it works now on Windows I don't think we need to concern ourselves with temporary files at all.
The odd thing with newline after wrong indentation seems to be Windows specific. I'd say we leave this out as out-of-scope for this issue.
I'll test this on other platforms after the weekend, but I'd say in this current version this could be merged already. But I would recommend to try out the two points I brought up earlier:
except Exception as e
: Would add more transparency to the user if the error message encountered when loading code from KQC was somehow displayed, unless the spam is too muchrestore_pcells_to_views
could be skipped if an error is encountered? If its more convenient to delete instances no matter what the outcome of load_libraries
, that's fine by me too as a first version of this feature.Thank you for the nice comments. I have written a more informative error message and made some minor modifications. I found out that it was necessary to delete the cells before reloading the libraries, otherwise they would lose the references to their children's instances, which didn't get deleted and appeared out of the top cell afterwards.
The current behavior on Linux is:
Current limitations:
Element
s after reloading and cannot be modified according to the libraries.
This pull request addresses issue #89. To start, I implemented a basic solution by making
extract_pcell_data_from_views
work only with instances ofElement
and by restoring the view in case of an exception when reloading the libraries.