Add option to drop filtered modes #3055
Conversation
momchil-flex
force-pushed
the
momchil/drop_filtered_modes
branch
from
87f4819 to
636f5c9
Compare
|
@momchil-flex thanks for this! I think in terms of API, I would prefer a field If you want I can take over this PR and make the change |
|
I like the suggestion in that the mode spec would still always produce a deterministic number of modes. But in terms of functionality, doesn't it sound not adequate e.g. for the EME filtering? Like if you strictly want to drop modes that don't meet a given criterion, this is not going to achieve that, or would achieve in some cases by chance? Maybe you're imagining having a separate filtering done in EME, which I think could make sense. |
It’s the closest you can easily get to deterministic number of modes, although not perfect. The sim coeffs can tell you how many modes were actually kept; if it’s fewer than the number requested, then you may need to increase num_modes for the computation, or loosen the filter |
|
Ok yea go for it. |
Maybe a field “keep_modes” which can be “all”, “filtered”, or an integer |
caseyflex
force-pushed
the
momchil/drop_filtered_modes
branch
2 times, most recently
from
7bd98b6 to
86fea1d
Compare
caseyflex
force-pushed
the
momchil/drop_filtered_modes
branch
from
86fea1d to
f618f6b
Compare
caseyflex
left a comment
caseyflex
left a comment
There was a problem hiding this comment.
@momchil-flex I kept support for your method via keep_modes="filtered".
I also added the option to have keep_modes be an integer, in which case it just keeps that many modes regardless of filtering, warning however if it keeps some modes that didn't pass the filer.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| raise ValidationError( | ||
| "ModeSortSpec.keep_modes cannot be larger than 'num_modes' ." | ||
| f"Currently these are {val.keep_modes} and {num_modes}." | ||
| ) |
There was a problem hiding this comment.
style: The error message could be more descriptive. Consider explaining the relationship between these parameters more clearly for better user experience.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: tidy3d/components/mode_spec.py
Line: 611:614
Comment:
**style:** The error message could be more descriptive. Consider explaining the relationship between these parameters more clearly for better user experience.
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
Diff CoverageDiff: origin/develop...HEAD, staged and unstaged changes
Summary
tidy3d/components/data/monitor_data.py |
|
Sorry, accidentally closed this. Sounds good for your change, I guess we'll work on finalizing this after the holidays but good for testing for now! |
| lower, upper = bounding_box.bounds | ||
| lower_bound = lower[normal_axis] | ||
| upper_bound = upper[normal_axis] | ||
| tol = 1e-9 |
There was a problem hiding this comment.
style: hardcoded tolerance value 1e-9 should be defined as a named constant
| tol = 1e-9 | |
| tol = fp_eps |
Context Used: Rule from dashboard - Avoid hardcoding values ("magic numbers") that can be programmatically derived from data; use named ... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: tidy3d/components/data/monitor_data.py
Line: 771:771
Comment:
**style:** hardcoded tolerance value `1e-9` should be defined as a named constant
```suggestion
tol = fp_eps
```
**Context Used:** Rule from `dashboard` - Avoid hardcoding values ("magic numbers") that can be programmatically derived from data; use named ... ([source](https://app.greptile.com/review/custom-context?memory=6661caa7-319d-4caf-8111-723b32818d62))
How can I resolve this? If you propose a fix, please make it concise.| num_modes = values.get("num_modes") | ||
| if val.keep_modes > num_modes: | ||
| raise ValidationError( | ||
| "ModeSortSpec.keep_modes cannot be larger than 'num_modes' ." |
There was a problem hiding this comment.
syntax: extra space before period in error message
| "ModeSortSpec.keep_modes cannot be larger than 'num_modes' ." | |
| "ModeSortSpec.keep_modes cannot be larger than 'num_modes'." |
Prompt To Fix With AI
This is a comment left during a code review.
Path: tidy3d/components/mode_spec.py
Line: 612:612
Comment:
**syntax:** extra space before period in error message
```suggestion
"ModeSortSpec.keep_modes cannot be larger than 'num_modes'."
```
How can I resolve this? If you propose a fix, please make it concise.| keep_modes = getattr(sort_spec, "keep_modes", False) | ||
| if keep_modes == "filtered" and sort_spec.filter_key is None: | ||
| raise ValidationError("ModeSortSpec.keep_modes requires 'filter_key' to be set.") |
There was a problem hiding this comment.
logic: duplicate validation - this validation is already performed by the pydantic validator _drop_requires_filter in ModeSortSpec at mode_spec.py:112-117, so it will never execute here
Context Used: Rule from dashboard - When modifying a piece of logic, ensure the change is propagated to all independent functions or cal... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: tidy3d/components/data/monitor_data.py
Line: 2520:2522
Comment:
**logic:** duplicate validation - this validation is already performed by the pydantic validator `_drop_requires_filter` in `ModeSortSpec` at mode_spec.py:112-117, so it will never execute here
**Context Used:** Rule from `dashboard` - When modifying a piece of logic, ensure the change is propagated to all independent functions or cal... ([source](https://app.greptile.com/review/custom-context?memory=809457d4-6b33-46ed-b49b-37057d02807e))
How can I resolve this? If you propose a fix, please make it concise.| masks_after.append(mask) | ||
|
|
||
| keep_mask = np.all(np.stack(masks_after, axis=0), axis=0) | ||
| if keep_modes == "filtered": |
There was a problem hiding this comment.
logic: keep_mask used here but only defined inside the conditional block above. if keep_modes == "all", this line would cause UnboundLocalError
| if keep_modes == "filtered": | |
| elif keep_modes == "filtered": |
Prompt To Fix With AI
This is a comment left during a code review.
Path: tidy3d/components/data/monitor_data.py
Line: 2622:2622
Comment:
**logic:** `keep_mask` used here but only defined inside the conditional block above. if `keep_modes == "all"`, this line would cause `UnboundLocalError`
```suggestion
elif keep_modes == "filtered":
```
How can I resolve this? If you propose a fix, please make it concise.| num_modes_sorted = filter_metric_sorted.sizes["mode_index"] | ||
| if keep_mask.sum() < num_modes_sorted: | ||
| keep_inds = np.where(keep_mask)[0] | ||
| elif isinstance(keep_modes, int): |
There was a problem hiding this comment.
logic: keep_mask used here but only defined inside the conditional block. this causes UnboundLocalError when keep_modes is an integer but the check on line 2606 is False
Prompt To Fix With AI
This is a comment left during a code review.
Path: tidy3d/components/data/monitor_data.py
Line: 2631:2631
Comment:
**logic:** `keep_mask` used here but only defined inside the conditional block. this causes `UnboundLocalError` when `keep_modes` is an integer but the check on line 2606 is False
How can I resolve this? If you propose a fix, please make it concise.| if keep_modes == "filtered": | ||
| if not np.any(keep_mask): | ||
| raise ValidationError( | ||
| "Filtering removes all modes; relax the filter threshold or change 'keep_modes'." | ||
| ) | ||
|
|
||
| num_modes_sorted = filter_metric_sorted.sizes["mode_index"] | ||
| if keep_mask.sum() < num_modes_sorted: | ||
| keep_inds = np.where(keep_mask)[0] | ||
| elif isinstance(keep_modes, int): | ||
| if keep_mask.sum() < keep_modes: | ||
| log.warning( | ||
| f"Filtering with 'keep_modes={keep_modes}' keeps " | ||
| f"more than the '{keep_mask.sum()}' modes which pass the filter. Consider " | ||
| "relaxing the filter threshold or changing 'keep_modes'." | ||
| ) | ||
| keep_inds = np.arange(keep_modes) |
There was a problem hiding this comment.
logic: indentation error causes incorrect control flow. lines 2622-2638 should be indented to be inside the if keep_modes == "filtered" or isinstance(keep_modes, int): block that starts on line 2606, since they reference keep_mask which is only defined within that block. currently, if keep_modes == "all", line 2622 would attempt to access undefined keep_mask
| if keep_modes == "filtered": | |
| if not np.any(keep_mask): | |
| raise ValidationError( | |
| "Filtering removes all modes; relax the filter threshold or change 'keep_modes'." | |
| ) | |
| num_modes_sorted = filter_metric_sorted.sizes["mode_index"] | |
| if keep_mask.sum() < num_modes_sorted: | |
| keep_inds = np.where(keep_mask)[0] | |
| elif isinstance(keep_modes, int): | |
| if keep_mask.sum() < keep_modes: | |
| log.warning( | |
| f"Filtering with 'keep_modes={keep_modes}' keeps " | |
| f"more than the '{keep_mask.sum()}' modes which pass the filter. Consider " | |
| "relaxing the filter threshold or changing 'keep_modes'." | |
| ) | |
| keep_inds = np.arange(keep_modes) | |
| if keep_modes == "filtered": | |
| if not np.any(keep_mask): | |
| raise ValidationError( | |
| "Filtering removes all modes; relax the filter threshold or change 'keep_modes'." | |
| ) | |
| num_modes_sorted = filter_metric_sorted.sizes["mode_index"] | |
| if keep_mask.sum() < num_modes_sorted: | |
| keep_inds = np.where(keep_mask)[0] | |
| elif isinstance(keep_modes, int): | |
| if keep_mask.sum() < keep_modes: | |
| log.warning( | |
| f"Filtering with 'keep_modes={keep_modes}' keeps " | |
| f"more than the '{keep_mask.sum()}' modes which pass the filter. Consider " | |
| "relaxing the filter threshold or changing 'keep_modes'." | |
| ) | |
| keep_inds = np.arange(keep_modes) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: tidy3d/components/data/monitor_data.py
Line: 2622:2638
Comment:
**logic:** indentation error causes incorrect control flow. lines 2622-2638 should be indented to be inside the `if keep_modes == "filtered" or isinstance(keep_modes, int):` block that starts on line 2606, since they reference `keep_mask` which is only defined within that block. currently, if `keep_modes == "all"`, line 2622 would attempt to access undefined `keep_mask`
```suggestion
if keep_modes == "filtered":
if not np.any(keep_mask):
raise ValidationError(
"Filtering removes all modes; relax the filter threshold or change 'keep_modes'."
)
num_modes_sorted = filter_metric_sorted.sizes["mode_index"]
if keep_mask.sum() < num_modes_sorted:
keep_inds = np.where(keep_mask)[0]
elif isinstance(keep_modes, int):
if keep_mask.sum() < keep_modes:
log.warning(
f"Filtering with 'keep_modes={keep_modes}' keeps "
f"more than the '{keep_mask.sum()}' modes which pass the filter. Consider "
"relaxing the filter threshold or changing 'keep_modes'."
)
keep_inds = np.arange(keep_modes)
```
How can I resolve this? If you propose a fix, please make it concise.
drop_modesboolean, ifTrue, just drop modes that don't make the filtering threshold. The caveat here is that the number of modes might depend on the frequency. The choice that was made was to do the dropping after the tracking, and then just do the intersection and keep the smallest number of modes that meet the filtering criterion at every frequency.fill_fraction_boxas a filtering key which then requires abounding_boxto be passed to thesort_spec. Also afill_fraction(box)method to the mode solver data, as well as a propertyfill_fraction_boxthat uses the box defined in the storedmode_spec.sort_spec.Greptile Overview
Greptile Summary
This PR adds functionality to drop modes that don't meet filtering thresholds and introduces a new
fill_fraction_boxfiltering metric for mode solvers.Major Changes:
keep_modesparameter toModeSortSpecwhich can be set to"filtered"to drop modes not passing the filter, or an integer to keep only the first N modesfill_fraction_boxas a new filtering/sorting key that computes the field-energy fill fraction within a specified bounding boxbounding_boxfield toModeSortSpecrequired when usingfill_fraction_boxfill_fraction()method andfill_fraction_boxproperty onModeSolverData_apply_mode_subset()helper method to extract a subset of modes after filteringIssues Found:
sort_modesat monitor_data.py:2622-2638 will causeUnboundLocalErrorwhenkeep_modes == "all"(the default case)Confidence Score: 1/5
keep_modes == "all"(the default), the code attempts to accesskeep_maskwhich is undefined outside theifblock on line 2606, causingUnboundLocalError. This affects the default behavior and will break existing functionality.sort_modesmethodImportant Files Changed
File Analysis
keep_modesfield andfill_fraction_boxfilter key with validators. Minor syntax error in validation message.fill_fractioncomputation and mode dropping logic. Critical indentation bug insort_modescausingUnboundLocalErrorwith certainkeep_modesvalues. Also contains duplicate validation and hardcoded tolerance.drop_modesfunctionality andfill_fraction_boxfilter. Test coverage is thorough including edge cases.Sequence Diagram
sequenceDiagram participant User participant ModeSortSpec participant ModeSolverData participant sort_modes participant fill_fraction_box User->>ModeSortSpec: Create with keep_modes="filtered" or int ModeSortSpec->>ModeSortSpec: Validate filter_key required ModeSortSpec->>ModeSortSpec: Validate bounding_box for fill_fraction_box User->>ModeSolverData: Call sort_modes(sort_spec) ModeSolverData->>sort_modes: Execute sorting logic alt filter_key == "fill_fraction_box" sort_modes->>fill_fraction_box: Compute fill fraction metric fill_fraction_box->>fill_fraction_box: Validate bounding box intersection fill_fraction_box->>fill_fraction_box: Create mask within bounding box fill_fraction_box->>fill_fraction_box: Calculate weighted intensity ratio fill_fraction_box-->>sort_modes: Return fill fraction values else other filter_key sort_modes->>ModeSolverData: Get metric (n_eff, k_eff, etc) end sort_modes->>sort_modes: Filter modes by threshold (per frequency) sort_modes->>sort_modes: Sort modes within groups sort_modes->>sort_modes: Track modes across frequencies (if enabled) alt keep_modes == "filtered" sort_modes->>sort_modes: Re-evaluate filter after tracking sort_modes->>sort_modes: Keep only modes passing filter at ALL freqs sort_modes->>sort_modes: Call _apply_mode_subset sort_modes->>sort_modes: Update num_modes to filtered count else keep_modes == int sort_modes->>sort_modes: Re-evaluate filter after tracking sort_modes->>sort_modes: Keep first keep_modes modes sort_modes->>sort_modes: Call _apply_mode_subset sort_modes->>sort_modes: Update num_modes to keep_modes end sort_modes-->>ModeSolverData: Return sorted/filtered data ModeSolverData-->>User: Return new ModeSolverData with updated modes