Skip to content

Conversation

@shanto268
Copy link

@shanto268 shanto268 commented Nov 18, 2024

What are the issues this pull addresses (issue numbers / links)?

Porting Qiskit Metal from PySide2 to PySide6 so that it can work natively on M* Macs (Apple Silicon).

Issues addressed:

Did you add tests to cover your changes (yes/no)?

No. N/A

Did you update the documentation accordingly (yes/no)?

Yes

Did you read the CONTRIBUTING document (yes/no)?

Yes

Summary

Fixed changes from PR #908 to pass automated CI workflows and tests, ensuring compatibility with updated dependencies, environment configurations, and the latest (11/17/2024) main branch.

Details and comments

  1. Built on top of changes made by @obrienpja in PR #908.

  2. Changes to QWidget_PlaceholderText and QTableView_AllComponents:

    • Refactored the QWidget_PlaceholderText class to ensure proper initialization and compatibility with PySide6.
    • Resolved issues with placeholder text styling by updating palette usage to align with PySide6 standards.
    • Updated QTableView_AllComponents to correctly inherit and integrate with QWidget_PlaceholderText, ensuring both functionality and visual styling work seamlessly.
    • Fixed initialization errors and ensured that the placeholder label behaves as intended when no components are available.
  3. Requirements and environment updates:

    • Updated the environment.yml file to use stable, OS-agnostic dependencies for Python 3.10.
    • Added missing dependencies, such as pyaedt, to ensure a consistent build across different operating systems.
    • Ensured that all dependencies align with the workflows and requirements in the lab's testing environment.
  4. Testing and validation:

    • Verified that the changes integrate seamlessly with the existing workflows and CI processes used in our lab.
    • Tested the updated branch against the current main branch workflows to confirm identical behavior.
    • All CI tests now pass successfully across all supported platforms (Windows, macOS, Ubuntu) and Python versions (3.9 and 3.10).

@CLAassistant
Copy link

CLAassistant commented Nov 18, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ obrienpja
✅ priti-ashvin-shah-ibm
❌ shanto268
You have signed the CLA already but the status is still pending? Let us recheck it.

@zlatko-minev
Copy link
Collaborator

Thank you, @shanto268! The tests are running. This is a giant PR, so it would be good to have more eyes on it than just you and me. Has anyone else from your lab tried this? I think after merging this, we could bump the Qiskit Metal version from 0.1.5 to 0.2. I just want to make sure it runs.

@SamWolski
Copy link

At first glance it looks like one of the entries in the Series is a list with a single Polygon object, rather than the Polygon object itself.

It has to be a list, on the next line, the * is unroling the list

Not quite. The * is unrolling a pandas Series, which should contain the geometry objects directly, but one of the Series entries is itself a list.

I figured it out though: the handling of geometry with interior holes was not ported to gdstk entirely correctly in QGDSRenderer._qgeometry_to_gds. Previously, gdspy.boolean was done with a PolygonSet which resulted in another PolygonSet which is legal to add to the GDS cell. Now, it is gdstk.boolean done with a list of polygons, which returns a list of polygons, which is added to the geometry table etc., hence the error.

The easiest fix I could think of is to return the first (and only) element of this list, ie return result[0] instead of return result. There should in theory only ever be a single element in result as we are only dealing with geometry with full interior holes, rather than geometry which could be split up into multiple disjoint segments by the boolean operation.

The return statement in question is here (line 2339):
https://github.com/shanto268/qiskit-metal/blob/bbc7d29b8225d5476294585b9740dd0ba82a3324/qiskit_metal/renderers/renderer_gds/gds_renderer.py#L2339C29-L2339C29

If this works for everyone, someone with write access can make the change directly (@shanto268 ?). 3 characters doesn't seem worth it for a PR.

@SamWolski
Copy link

In any case, I don't think it's a good idea to release a large and long-awaited update to Qiskit Metal, which might I remind everyone is explicitly advertised as "enables native support for M* Macs (Apple Silicon)", as requiring non-trivial user intervention post-install to enable the fundamental functionality of the package itself.

I did not mean to ask, in the next release of qiskit-metal, the Mac user to setup this env variable. I meant that I do not see why PyAedt would not agree to replace the occurences of "Linux" by "posix" since it does not change anything from their side, while enabling Mac users to use some depending libraries on the other side. Thus, setting this variable is a short fix in the meantime (I guess they will release a new version of it before qiskit-metal).

I asked for clarification on the pyaedt side here: ansys/pyaedt#6704

@PositroniumJS
Copy link
Contributor

I have submitted a new PR shanto268#7 to @shanto268 that applies our two remarks @SamWolski
I have also updated the workflows:

  • on Mac, it is failing because of the issue from pyaedt as raised above
  • on Windows and Linux, there is one test that is failing, I did not have time to dig into it

@PositroniumJS
Copy link
Contributor

@shanto268 shanto268#8 fixes a last issue I have found in QGDSRenderer. It also fixes the tests. The Mac tests will be fixed in the next release of pyaedt, we will need at this moment to bump the dependency version then everything should be fine for merging @zlatko-minev

@PositroniumJS
Copy link
Contributor

pyaedt has been updated, it can be imported again on Mac devices.
Hence, the last tests are fixed in shanto268#9.
@shanto268 @zlatko-minev

@zlatko-minev
Copy link
Collaborator

zlatko-minev commented Oct 10, 2025 via email

…rt of structures with large fillet

Modify the fix commit fa54459
@SamWolski
Copy link

Cheesing appears to be broken when rendering to GDS. It can be bypassed as a workaround by selectively enabling/disabling the cheese and no-cheese layers:

design.renderers.gds.options.update({
	"cheese": {"view_in_file": {"main": {1: False},}},
	"no_cheese": {"view_in_file": {"main": {1: True}}},
})

but then there is obviously no cheesing generated.

I've also noticed that the cell and layer structure of the generated GDS file has changed. The ground_main_1 cell and 1/0 layer used to correspond to the ground plane NOT including any central conductors, but they are both gone. The layer has now been merged into 1/10. I haven't had a chance to look into it more deeply, but I suspect the cheesing algorithm is trying to find the missing cell and/or layer, at which point the code crashes.

The merging of the ground plane and central conductor elements is problematic for those of us who need to do our own cheesing and other post-processing, as the ground plane needs to be treated differently to other metallic elements like central conductors.

@PositroniumJS
Copy link
Contributor

I've also noticed that the cell and layer structure of the generated GDS file has changed. The ground_main_1 cell and 1/0 layer used to correspond to the ground plane NOT including any central conductors, but they are both gone. The layer has now been merged into 1/10. I haven't had a chance to look into it more deeply, but I suspect the cheesing algorithm is trying to find the missing cell and/or layer, at which point the code crashes.

I am not using the cheesing but I had made some comits to port it to gdstk earlier on this PR, so it should still be working I guess. In any case, in the GDS that I am making, I still have the layer 1/0. Are you doing something special? 🤔

@zlatko-minev
Copy link
Collaborator

Thank you @shanto268 @PositroniumJS and @SamWolski , I feel we are on good track here. Excited to get close in the next week or two to merge this super-mega pull request and version update into main.

We will then advertise in the Quantum device community and have more people who would like to test it and try it out. After a few small patches, and when we feel it's in a stable in a version that it could be used in a tutorials for next year's quantum device workshop, we can tag it and release it as unofficial new version across the distribution channels. This will be a big moment for the community, the community has been waiting this for maybe two years.

Of course, we will make further improvements for the Quantum device workshop tutorials down the line after that, but it will be good to have this in a stable enough place.

@SamWolski
Copy link

SamWolski commented Oct 22, 2025

I am not using the cheesing but I had made some comits to port it to gdstk earlier on this PR, so it should still be working I guess.

Clean python env created with conda, only qiskit-metal (either main or this PR) installed:

conda create -n <env_name> python=3.10
conda activate <env_name>
pip install <path_to_cloned_repo>

Generate a minimal design that will actually export something:

import qiskit_metal as metal
from qiskit_metal.qlibrary.terminations.launchpad_wb import LaunchpadWirebond

design = metal.designs.DesignPlanar()
_ = LaunchpadWirebond(design, "pad_1", options={},)
design.renderers.gds.export_to_gds("new_metal_test.gds")

On main, this is successful and generates the GDS file as expected.
On this PR, this fails with

  File "<env>/lib/python3.10/site-packages/qiskit_metal/renderers/renderer_gds/make_cheese.py", line 443, in _subtract_holes_from_ground
    ground_cell = next(
StopIteration

@SamWolski
Copy link

In any case, in the GDS that I am making, I still have the layer 1/0. Are you doing something special? 🤔

If I run the same minimal script, but bypass the cheesing just before the export with

design.renderers.gds.options.update({
	"cheese": {"view_in_file": {"main": {1: False},}},
	"no_cheese": {"view_in_file": {"main": {1: True}}},
})

The main version creates layers 1/0, 1/10, and 1/99, whereas the PR version generates only 1/10 and 1/99, with the ground plane merged into the 1/10 layer.

@SamWolski
Copy link

I've managed to solve both the issues for positive masks; it's just a few lines that were dropped in the port to gdstk. I'll give it my best shot for negative masks and PR my fork with the fixes.

Before porting from gdspy to gdstk, the ground plane for positive masks had a dedicated cell (eg. ground_main_1) and layer with datatype 0.
This was lost during the port, and is restored here.

This functionality allows the ground plane to be easily differentiated from other metallized regions for custom post-processing after gds export, such as adding high-density cheesing.
@SamWolski
Copy link

Just verified, no changes need to be made for the negative mask (no separate ground plane layer was ever created, and the generated files appear to match). PR is submitted.

# User Folders:
.scrapy
_sandbox_/
_debug/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_debug/

# virtualenv
.venv
venv/
venvz/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
venvz/
venvz/

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.