-
Notifications
You must be signed in to change notification settings - Fork 52
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
Add periodic repetition in EME simulations #2331
base: develop
Are you sure you want to change the base?
Conversation
faeaa29
to
80f8088
Compare
01ee8e5
to
f51beb5
Compare
That's great @caseyflex . At the conference this week so hard to find a good chuck of time to carefully review it. Maybe @momchil-flex can take a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @caseyflex, the code looks good to me, no comments there, only some high level ones regarding documentation.
This might just be my unfamiliarity with EME but from reading through the PR it wasn't really obvious to me when and how you would use this functionality, and for example what is the difference between a EMEModeSpec(num_reps=N)
and an EMEPeriodicitySweep
? A more detailed docstring at least for the latter would help greatly I think. Also perhaps a quick motivation in the docstring ("Periodic repetition [...] for X purpose") would go a long way. Is there a related example notebook?
Ah thanks @yaugenst-flex , I forgot the docstring. I'll add some details there. I think @tomflexcompute will make an example notebook once he has a bit of time. Currently the api is like this:
this defines a composite grid with two port subgrids (each having one cell) and a unit cell subgrid consisting of two cells which is meant to be periodically repeated, see the image below. The unit cell grid is named Then there are two main ways you could use this. You could just set
then this would simulate three different structures at once, with varying number of repetitions of the unit cell. Let me know your thoughts on the API! |
f51beb5
to
aa846fa
Compare
aa846fa
to
abb1f83
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for that explanation @caseyflex, that's really helpful, and the interface makes sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @caseyflex for the great addition of the EME capability!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @caseyflex , just some small comments/questions
cls, structures: List[Structure], axis: Axis, mode_spec: EMEModeSpec, **kwargs | ||
) -> EMEExplicitGrid: | ||
"""Create an explicit EME grid with boundaries aligned with | ||
structure bounding boxes.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is user-facing it should have a proper doc string with parameters and return, and maybe an example.
return self | ||
|
||
@property | ||
def cell_index_pairs(self) -> List[pd.NonNegativeInt]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this and updated_copy_num_reps
be private? Or is there a reason we might want them user-exposed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, fair
num_reps: List[pd.PositiveInt] = None, | ||
) -> EMECompositeGrid: | ||
"""Create a composite EME grid with boundaries aligned with | ||
structure bounding boxes.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about full docstring.
f"Monitor at 'monitors[{i}]' is an 'EMEFieldMonitor', " | ||
"which is not compatible with periodic repetition " | ||
"('num_reps != 1' in any 'EMEGridSpec'.)" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to lift this restriction eventually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because the thing is the field may vary cell to cell on the repeated grid, so there is no way of capturing it on the original EME grid (before repetition)
Example | ||
------- | ||
>>> n_list = [1, 50, 100] | ||
>>> sweep_spec = EMEPeriodicitySweep(num_reps=[{"unit_cell": n} for n in n_list]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how I feel about using the name
attribute for functional purposes. But this is not the first place in the code this happens. Just curious - is it used anywhere else in EME, and did you consider any alternatives?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s only here. I considered forcing the sweep num_reps to be a list of tuples, each of which contained a num_reps for the current grid spec, as well as a list of num_reps for the sub grids if it is composite (as a recursive data structure in the same format as the grid spec). But this was way too cumbersome
Btw the current limit on the number of frequencies is still a little limited I find. Just wondering if the limit is due to cost consideration? The credit cost scales linearly with number of frequencies right? |
@momchil-flex it was actually just based on simulations taking too long, I remember you weighed in on setting it to 20, shall we relax it to maybe 500? |
Major changes:
num_reps
to everyEMEGridSpec
class, which can be used to periodically repeat that subgrid while reusing mode solves, overlaps, and interface s-matricesEMEPeriodicitySweep
which allows sweeping thenum_reps
of everyEMEGridSpec
in theEMESimulation.eme_grid_spec
. This is currently done by assigning names toEMEGridSpec
objects, and then passingnum_reps
as a list of dictionaries, one for each sweep_index, and each of which is a mapping from the name of the subgrid which will be repeated to the desired number of repetitions of that subgrid.Minor changes:
EMEExplicitGrid.from_structures
andEMECompositeGrid.from_structure_groups
which place grid boundaries at structure boundsMAX_NUM_FREQS
to 100Todo: