Skip to content
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.

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

fix: GDS export wrappers in JaxSimulation (#1334) #1660

Merged
merged 2 commits into from
May 2, 2024

Conversation

yaugenst-flex
Copy link
Contributor

Fixes #1334 by overriding to_gds() in JaxSimulation to call self.to_simulation() first. Addresses both to_gds() and to_gds_file() this way.

@yaugenst-flex yaugenst-flex marked this pull request as draft May 2, 2024 14:17
Copy link
Collaborator

@tylerflex tylerflex left a comment

Choose a reason for hiding this comment

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

just took a quick look, could we also add

  1. the other gds conversion methods (I think there's also to_gds_file(), might be one or two more but that's the main one people will use I think).
  2. Just copy over the docstring from the original methods so people can see the parameters (I realize this is kind of ugly but it's what we have until we can better refactor things).
  3. Add a line in the changelog under "added" in "unreleased"

Thanks!

@yaugenst-flex
Copy link
Contributor Author

Hey @tylerflex, to_gds_file() works and is actually what's being tested, since it calls to_gds() internally. I'll add the docstring and changelog notes.

@yaugenst-flex
Copy link
Contributor Author

Ah, but to_gdstk() and to_gdspy() probably won't work like this, you're right.

@tylerflex
Copy link
Collaborator

oh I see. hm, maybe we dont need the specific gdstk and gdspy ones but if you're already working on them, it wouldn't hurt. Good to know that the to_gds_file will work automatically, I guess just mention all of them that will work from JaxSimulation now in the changelog and should be good.

@yaugenst-flex yaugenst-flex marked this pull request as ready for review May 2, 2024 16:42
Copy link
Collaborator

@tylerflex tylerflex left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks @yaugenst-flex , just a couple minor comments and also please squash the commits to 1 (or 2 maybe with one of them to fix the gds{py, tk} methods) if possible). Thanks!

tidy3d/plugins/adjoint/components/simulation.py Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@yaugenst-flex
Copy link
Contributor Author

please squash the commits to 1 (or 2 maybe with one of them to fix the gds{py, tk} methods) if possible)

Done!

Copy link
Collaborator

@tylerflex tylerflex left a comment

Choose a reason for hiding this comment

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

Looks great! thanks @yaugenst-flex . I will merge once the tests pass!

@tylerflex tylerflex merged commit 1fe777b into pre/2.7 May 2, 2024
16 checks passed
@tylerflex tylerflex deleted the yaugenst-flex/issue1334 branch May 2, 2024 18:52
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.

2 participants