[experimental] Curved-element meshing and Palace 3D photonics example #131
Open
mdmaas wants to merge 7 commits into
Open
[experimental] Curved-element meshing and Palace 3D photonics example #131mdmaas wants to merge 7 commits into
mdmaas wants to merge 7 commits into
Conversation
mdmaas
changed the title
mdmaas
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for patterned dielectric layers and high-order geometric mesh elements in Palace simulations. Key features include the ability to disable synthetic dielectric regions, the implementation of spline and bspline curve fitting for dielectric boundaries, and the integration of high-order elements into the meshing and visualization pipelines. A new example notebook for a directional coupler is provided; however, feedback indicates that the current validation logic incorrectly triggers a ValueError for purely dielectric simulations, which is a critical issue for photonic component modeling.
Comment on lines
+309
to
+333
| "cell_type": "code", | ||
| "execution_count": 7, | ||
| "id": "59a9840b", | ||
| "metadata": {}, | ||
| "outputs": [ | ||
| { | ||
| "ename": "ValueError", | ||
| "evalue": "Mesh validation failed:\nValidation: FAILED\nErrors:\n - No conductor surfaces in mesh. Check that conductor layers have polygons and correct layer_type.\n - config.json has no Conductivity or PEC boundaries.\nWarnings:\n - Volumes: ['air', 'core']", | ||
| "output_type": "error", | ||
| "traceback": [ | ||
| "\u001b[31m---------------------------------------------------------------------------\u001b[39m", | ||
| "\u001b[31mValueError\u001b[39m Traceback (most recent call last)", | ||
| "\u001b[36mCell\u001b[39m\u001b[36m \u001b[39m\u001b[32mIn[7]\u001b[39m\u001b[32m, line 1\u001b[39m\n\u001b[32m----> \u001b[39m\u001b[32m1\u001b[39m results = sim.run()\n", | ||
| "\u001b[36mFile \u001b[39m\u001b[32m~/Desktop/gsim/src/gsim/palace/driven.py:128\u001b[39m, in \u001b[36mDrivenSim.run\u001b[39m\u001b[34m(self, parent_dir, verbose, wait)\u001b[39m\n\u001b[32m 115\u001b[39m \u001b[38;5;250m\u001b[39m\u001b[33;03m\"\"\"Run the driven sim on GDSFactory+ cloud.\u001b[39;00m\n\u001b[32m 116\u001b[39m \n\u001b[32m 117\u001b[39m \u001b[33;03mThin wrapper over :meth:`PalaceSimMixin.run` that narrows the\u001b[39;00m\n\u001b[32m (...)\u001b[39m\u001b[32m 124\u001b[39m \u001b[33;03m ``job_id`` string when ``wait=False``.\u001b[39;00m\n\u001b[32m 125\u001b[39m \u001b[33;03m\"\"\"\u001b[39;00m\n\u001b[32m 126\u001b[39m \u001b[38;5;28;01mfrom\u001b[39;00m\u001b[38;5;250m \u001b[39m\u001b[34;01mgsim\u001b[39;00m\u001b[34;01m.\u001b[39;00m\u001b[34;01mpalace\u001b[39;00m\u001b[34;01m.\u001b[39;00m\u001b[34;01mresults\u001b[39;00m\u001b[38;5;250m \u001b[39m\u001b[38;5;28;01mimport\u001b[39;00m SParams \u001b[38;5;28;01mas\u001b[39;00m _SParams\n\u001b[32m--> \u001b[39m\u001b[32m128\u001b[39m result = \u001b[30;43msuper\u001b[39;49m\u001b[30;43m(\u001b[39;49m\u001b[30;43m)\u001b[39;49m\u001b[30;43m.\u001b[39;49m\u001b[30;43mrun\u001b[39;49m\u001b[30;43m(\u001b[39;49m\u001b[30;43mparent_dir\u001b[39;49m\u001b[30;43m,\u001b[39;49m\u001b[30;43m \u001b[39;49m\u001b[30;43mverbose\u001b[39;49m\u001b[30;43m=\u001b[39;49m\u001b[30;43mverbose\u001b[39;49m\u001b[30;43m,\u001b[39;49m\u001b[30;43m \u001b[39;49m\u001b[30;43mwait\u001b[39;49m\u001b[30;43m=\u001b[39;49m\u001b[30;43mwait\u001b[39;49m\u001b[30;43m)\u001b[39;49m\n\u001b[32m 129\u001b[39m \u001b[38;5;28;01mif\u001b[39;00m \u001b[38;5;28misinstance\u001b[39m(result, (_SParams, \u001b[38;5;28mstr\u001b[39m)):\n\u001b[32m 130\u001b[39m \u001b[38;5;28;01mreturn\u001b[39;00m result\n", | ||
| "\u001b[36mFile \u001b[39m\u001b[32m~/Desktop/gsim/src/gsim/palace/base.py:1430\u001b[39m, in \u001b[36mPalaceSimMixin.run\u001b[39m\u001b[34m(self, parent_dir, verbose, wait)\u001b[39m\n\u001b[32m 1390\u001b[39m \u001b[38;5;28;01mdef\u001b[39;00m\u001b[38;5;250m \u001b[39m\u001b[34mrun\u001b[39m(\n\u001b[32m 1391\u001b[39m \u001b[38;5;28mself\u001b[39m,\n\u001b[32m 1392\u001b[39m parent_dir: \u001b[38;5;28mstr\u001b[39m | Path | \u001b[38;5;28;01mNone\u001b[39;00m = \u001b[38;5;28;01mNone\u001b[39;00m,\n\u001b[32m (...)\u001b[39m\u001b[32m 1395\u001b[39m wait: \u001b[38;5;28mbool\u001b[39m = \u001b[38;5;28;01mTrue\u001b[39;00m,\n\u001b[32m 1396\u001b[39m ) -> SParams | \u001b[38;5;28mdict\u001b[39m[\u001b[38;5;28mstr\u001b[39m, Path] | \u001b[38;5;28mstr\u001b[39m:\n\u001b[32m 1397\u001b[39m \u001b[38;5;250m \u001b[39m\u001b[33;03m\"\"\"Run simulation on GDSFactory+ cloud.\u001b[39;00m\n\u001b[32m 1398\u001b[39m \n\u001b[32m 1399\u001b[39m \u001b[33;03m Requires mesh() to be called first. Automatically calls\u001b[39;00m\n\u001b[32m (...)\u001b[39m\u001b[32m 1428\u001b[39m \u001b[33;03m >>> print(results[\"eig.csv\"])\u001b[39;00m\n\u001b[32m 1429\u001b[39m \u001b[33;03m \"\"\"\u001b[39;00m\n\u001b[32m-> \u001b[39m\u001b[32m1430\u001b[39m \u001b[30;43mself\u001b[39;49m\u001b[30;43m.\u001b[39;49m\u001b[30;43mupload\u001b[39;49m\u001b[30;43m(\u001b[39;49m\u001b[30;43mverbose\u001b[39;49m\u001b[30;43m=\u001b[39;49m\u001b[30;43;01mFalse\u001b[39;49;00m\u001b[30;43m)\u001b[39;49m\n\u001b[32m 1431\u001b[39m \u001b[38;5;28mself\u001b[39m.start(verbose=verbose != \u001b[33m\"\u001b[39m\u001b[33mquiet\u001b[39m\u001b[33m\"\u001b[39m)\n\u001b[32m 1432\u001b[39m \u001b[38;5;28;01mif\u001b[39;00m \u001b[38;5;129;01mnot\u001b[39;00m wait:\n", | ||
| "\u001b[36mFile \u001b[39m\u001b[32m~/Desktop/gsim/src/gsim/palace/base.py:1322\u001b[39m, in \u001b[36mPalaceSimMixin.upload\u001b[39m\u001b[34m(self, verbose)\u001b[39m\n\u001b[32m 1308\u001b[39m \u001b[38;5;250m\u001b[39m\u001b[33;03m\"\"\"Prepare config, upload to the cloud. Does NOT start execution.\u001b[39;00m\n\u001b[32m 1309\u001b[39m \n\u001b[32m 1310\u001b[39m \u001b[33;03mRequires :meth:`set_output_dir` and :meth:`mesh` to have been\u001b[39;00m\n\u001b[32m (...)\u001b[39m\u001b[32m 1318\u001b[39m \u001b[33;03m or :func:`gsim.wait_for_results`.\u001b[39;00m\n\u001b[32m 1319\u001b[39m \u001b[33;03m\"\"\"\u001b[39;00m\n\u001b[32m 1320\u001b[39m \u001b[38;5;28;01mfrom\u001b[39;00m\u001b[38;5;250m \u001b[39m\u001b[34;01mgsim\u001b[39;00m\u001b[38;5;250m \u001b[39m\u001b[38;5;28;01mimport\u001b[39;00m gcloud\n\u001b[32m-> \u001b[39m\u001b[32m1322\u001b[39m tmp = \u001b[30;43mself\u001b[39;49m\u001b[30;43m.\u001b[39;49m\u001b[30;43m_prepare_upload_dir\u001b[39;49m\u001b[30;43m(\u001b[39;49m\u001b[30;43m)\u001b[39;49m\n\u001b[32m 1323\u001b[39m \u001b[38;5;28;01mtry\u001b[39;00m:\n\u001b[32m 1324\u001b[39m \u001b[38;5;28mself\u001b[39m._job_id = gcloud.upload(tmp, \u001b[33m\"\u001b[39m\u001b[33mpalace\u001b[39m\u001b[33m\"\u001b[39m, verbose=verbose)\n", | ||
| "\u001b[36mFile \u001b[39m\u001b[32m~/Desktop/gsim/src/gsim/palace/base.py:1295\u001b[39m, in \u001b[36mPalaceSimMixin._prepare_upload_dir\u001b[39m\u001b[34m(self)\u001b[39m\n\u001b[32m 1292\u001b[39m \u001b[38;5;28;01mraise\u001b[39;00m \u001b[38;5;167;01mValueError\u001b[39;00m(\u001b[33m\"\u001b[39m\u001b[33mOutput directory not set. Call set_output_dir() first.\u001b[39m\u001b[33m\"\u001b[39m)\n\u001b[32m 1294\u001b[39m \u001b[38;5;66;03m# Always (re)generate config.json to reflect current driven settings\u001b[39;00m\n\u001b[32m-> \u001b[39m\u001b[32m1295\u001b[39m \u001b[30;43mself\u001b[39;49m\u001b[30;43m.\u001b[39;49m\u001b[30;43mwrite_config\u001b[39;49m\u001b[30;43m(\u001b[39;49m\u001b[30;43m)\u001b[39;49m\n\u001b[32m 1297\u001b[39m \u001b[38;5;66;03m# Copy input files to a temp dir so we don't destroy the user's directory\u001b[39;00m\n\u001b[32m 1298\u001b[39m tmp = Path(tempfile.mkdtemp(prefix=\u001b[33m\"\u001b[39m\u001b[33mpalace_\u001b[39m\u001b[33m\"\u001b[39m))\n", | ||
| "\u001b[36mFile \u001b[39m\u001b[32m~/Desktop/gsim/src/gsim/palace/base.py:1272\u001b[39m, in \u001b[36mPalaceSimMixin.write_config\u001b[39m\u001b[34m(self)\u001b[39m\n\u001b[32m 1270\u001b[39m validation = \u001b[38;5;28mself\u001b[39m.validate_mesh()\n\u001b[32m 1271\u001b[39m \u001b[38;5;28;01mif\u001b[39;00m \u001b[38;5;129;01mnot\u001b[39;00m validation.valid:\n\u001b[32m-> \u001b[39m\u001b[32m1272\u001b[39m \u001b[38;5;28;01mraise\u001b[39;00m \u001b[38;5;167;01mValueError\u001b[39;00m(\u001b[33mf\u001b[39m\u001b[33m\"\u001b[39m\u001b[33mMesh validation failed:\u001b[39m\u001b[38;5;130;01m\\n\u001b[39;00m\u001b[38;5;132;01m{\u001b[39;00mvalidation\u001b[38;5;132;01m}\u001b[39;00m\u001b[33m\"\u001b[39m)\n\u001b[32m 1274\u001b[39m \u001b[38;5;28;01mreturn\u001b[39;00m config_path\n", | ||
| "\u001b[31mValueError\u001b[39m: Mesh validation failed:\nValidation: FAILED\nErrors:\n - No conductor surfaces in mesh. Check that conductor layers have polygons and correct layer_type.\n - config.json has no Conductivity or PEC boundaries.\nWarnings:\n - Volumes: ['air', 'core']" | ||
| ] | ||
| } | ||
| ], | ||
| "source": [ | ||
| "results = sim.run()" | ||
| ] |
There was a problem hiding this comment.
This cell fails with a ValueError during mesh validation, specifically complaining about the absence of conductor surfaces. This seems to indicate that the validation logic in PalaceSimMixin.validate_mesh() doesn't correctly handle purely dielectric simulations, which is common for photonic components like this directional coupler.
Since this notebook serves as an example for the new curved-element meshing feature, it's critical that it runs successfully. The validation should likely be relaxed for simulations that only use wave ports on dielectric materials.
joamatab
approved these changes
May 17, 2026
Contributor
joamatab
left a comment
joamatab
left a comment
There was a problem hiding this comment.
LGTM - curved-element meshing and Palace 3D photonics
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR implements spline-fitting to replace curve loops, which enables curved-element meshing.
This is potentially interesting for several PDKs with curved features (like QPDK) but maybe most interesting for 3D photonics, for which Palace could lead to better performance than Meep.