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[invdes]: handle pixel size validation for simulations without sources #2161

Merged
merged 1 commit into from
Jan 15, 2025

Conversation

yaugenst-flex
Copy link
Collaborator

Currently InverseDesign validation will error if the simulation contains no sources due to the _check_pixel_size validator trying to access sim.wvl_mat_min, which depends on the sources being present.
This PR skips the validator if the simulation contains no sources and instead warns the user about checking the pixel size. The reason for not erroring is that we probably still want to allow the construction of simulations without sources in the plugin since we also allow it in td.Simulation (and it can be useful in some cases).

@yaugenst-flex yaugenst-flex self-assigned this Jan 14, 2025
@tylerflex
Copy link
Collaborator

Porting from other discussion: do we want to just fix sim.wvl_mat_min to use some default value even if sources are not defined? I think there was a related discussion about the RunTimeSpec not working with no sources present

@yaugenst-flex yaugenst-flex added the 2.8 will go into version 2.8.* label Jan 14, 2025
@yaugenst-flex
Copy link
Collaborator Author

@momchil-flex @tylerflex I think it might be best to raise an error in sim.wvl_mat_min in case sources are not present. Returning an arbitrary value seems incorrect. This fix would also work for the invdes plugin since the error message is propagated. So something along the lines of Simulation contains no sources, cannot determine wavelength in material would work fine.

Copy link

Copy link
Collaborator

@e-g-melo e-g-melo left a comment

Choose a reason for hiding this comment

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

Thanks @yaugenst-flex!

I just gave it a try, and it works well. In which cases do you plan to run optimizations without sources? Eigenmode optimizations?

Emerson

@yaugenst-flex
Copy link
Collaborator Author

I just gave it a try, and it works well. In which cases do you plan to run optimizations without sources? Eigenmode optimizations?

Great! Actually it's not as much about running the optimization but just being able to set up optimizations, because a common pattern we use involves doing base_something.updated_copy(field=new_items). It can also be useful when running locally to test the penalties maybe. But if we end up going for the sim.wvl_mat_min error then we'll error instead, which should also be fine. In that case we can give a good error message that there are no sources present, which should also be fine for GUI right?

@e-g-melo
Copy link
Collaborator

Great! Actually it's not as much about running the optimization but just being able to set up optimizations, because a common pattern we use involves doing base_something.updated_copy(field=new_items). It can also be useful when running locally to test the penalties maybe. But if we end up going for the sim.wvl_mat_min error then we'll error instead, which should also be fine. In that case we can give a good error message that there are no sources present, which should also be fine for GUI right?

Got it!

Yes, it should also work for GUI.

@yaugenst-flex
Copy link
Collaborator Author

Merging now but will follow up with a fix for sim.wvl_mat_min

@yaugenst-flex yaugenst-flex merged commit 405aef2 into pre/2.8 Jan 15, 2025
15 checks passed
@yaugenst-flex yaugenst-flex deleted the yaugenst-flex/invdes-source-validation branch January 15, 2025 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.8 will go into version 2.8.*
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants