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

lsf - added more monitor parsing capability #1184

Merged
merged 1 commit into from
Sep 30, 2023
Merged

Conversation

alec-flexcompute
Copy link
Collaborator

Added capability to parse in the following (marked in green) monitors from lsf. These are permittivity monitors, field time monitors, and field monitors expressed in different solvers. I currently have it so that monitors that record the same information but in different solver regimes (e.g. addindex vs addemeindex) route to the same function in the parser.

This should thus handle every monitor instance in lumerical for which there is a Tidy3D counterpart.

Also added monitor.lsf as an example lsf file for testing that includes an example of every monitor we can handle, and a patch-up handling cases where no e.g. sources, structures, monitors, and override structures are given.

image

@alec-flexcompute alec-flexcompute changed the title added more monitors capability lsf - added more monitor parsing capability Sep 28, 2023
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, let's just add a test for the new file. I also just tried this out and it worked as expected. thanks!

tests/data/monitors.lsf Outdated Show resolved Hide resolved
tidy3d/web/cli/converter.py Outdated Show resolved Hide resolved
@alec-flexcompute alec-flexcompute force-pushed the alec/lsf_parser branch 19 times, most recently from f319e42 to 55a2675 Compare September 29, 2023 03:30
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.

Thanks @alec-flexcompute, just a few things for refining the test. It would be good for you to learn some of these advanced pytest features (parameterize, tmp_path) as we use them throughout the code and they are really useful.

tests/test_package/test_convert.py Outdated Show resolved Hide resolved
tests/test_package/test_convert.py Outdated Show resolved Hide resolved
tests/test_package/test_convert.py Outdated Show resolved Hide resolved
tests/test_package/test_convert.py Outdated Show resolved Hide resolved
tests/test_package/test_convert.py Outdated Show resolved Hide resolved
tests/test_package/test_convert.py Outdated Show resolved Hide resolved
@alec-flexcompute alec-flexcompute force-pushed the alec/lsf_parser branch 4 times, most recently from 6b7a423 to 353827f Compare September 29, 2023 18:30
@tylerflex tylerflex changed the base branch from develop to pre/2.5 September 29, 2023 19:32
@tylerflex
Copy link
Collaborator

@alec-flexcompute we will merge this into pre/2.5. Could you please get it ready for that? (rebasing against the most up to date pre/2.5, fixing any conflicts that come up, single commit). Make sure to make a backup of the branch if you aren't comfortable with it, thanks!

@tylerflex tylerflex added the 2.5 label Sep 29, 2023
@alec-flexcompute alec-flexcompute force-pushed the alec/lsf_parser branch 2 times, most recently from a1aea73 to fe6586a Compare September 29, 2023 21:45
@alec-flexcompute
Copy link
Collaborator Author

Rebased to 2.5

@tylerflex
Copy link
Collaborator

There should be just one commit in this PR, could you double check that you pulled the most recent pre/2.5 branch before rebasing?

@tylerflex tylerflex changed the base branch from pre/2.5 to develop September 30, 2023 00:29
@alec-flexcompute alec-flexcompute force-pushed the alec/lsf_parser branch 3 times, most recently from 07efa48 to c717acf Compare September 30, 2023 00:41
@tylerflex tylerflex merged commit 93d0fcc into develop Sep 30, 2023
16 checks passed
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.

None yet

3 participants