-
Notifications
You must be signed in to change notification settings - Fork 70
feat: Unify handling of EME coefficients (FXC-4311) #3181
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
Conversation
336364d to
d27b9fc
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.
5 files reviewed, 1 comment
d27b9fc to
132076e
Compare
132076e to
f982375
Compare
Diff CoverageDiff: origin/develop...HEAD, staged and unstaged changes
Summary
|
f982375 to
7305ea0
Compare
momchil-flex
left a comment
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.
This is probably a feat not a chore :)
7305ea0 to
d118b2e
Compare
71a4b4c to
d947e26
Compare
|
@momchil-flex changed to a It changes the schema -- when should we merge it? |
|
Soon, hopefully before the end of the week we'll move the version to 2.11.0dev1. |
yaugenst-flex
left a comment
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 a couple clarifying questions
d947e26 to
d0975a4
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.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
This PR:
EMECoefficientMonitor.fieldswhich can be a list of some of "A", "B", "overlaps", "interface_smatrices", "n_complex", and "flux"EMESimulationData.coeffswhich is like the full monitor but with no downsampling.EMECoefficientMonitorsupports downsampling viaeme_cell_interval_space,num_sweep, andnum_modes.Note
Unifies EME coefficient handling and storage estimation while enabling selective coefficient capture.
EMECoefficientMonitor: addsfields("A", "B", "n_complex", "flux", "interface_smatrices", "overlaps");storage_size()rewritten to be field- and sweep-aware and to use virtual EME cellssweep_modes/interfaces/cellsflags on sweep specs; all EME monitorstorage_size()methods now acceptsweep_specand respect per-monitornum_sweeplimitscoeffs_full_monitorand includes it in_monitors_fullwhenstore_coeffs=True; removes manual coeff size math in favor of monitor-based sizingEMESimulationData.coeffsacceptsEMECoefficientData | EMECoefficientDatasetfieldsfields, zero-storage edge case, sweep-specific scaling, per-monitor limits, and inclusion logicWritten by Cursor Bugbot for commit d0975a4. This will update automatically on new commits. Configure here.
Greptile Summary
This PR unifies the handling of EME coefficients by adding a configurable
fieldsproperty toEMECoefficientMonitorthat supports the same fields asEMESimulationData.coeffs("A", "B", "n_complex", "flux", "interface_smatrices", "overlaps"). The default behavior remains unchanged withfields=("A", "B"), but users can now select additional fields with the benefit of downsampling viaeme_cell_interval_space,num_sweep, andnum_modes.Key changes:
EMECoefficientMonitor.fieldsproperty with comprehensive field selection optionsstore_coeffsandstore_port_modeswarningsEMESimulationData.coeffstype fromEMECoefficientDatasettoEMECoefficientDatafor consistency with monitor data architectureThe implementation correctly calculates storage sizes for each field type based on their respective dimensionalities (cells, modes, interfaces, sweeps).
Confidence Score: 4/5
coeffs_full_monitorthat states it returns a mode solver monitor when it actually returns a coefficient monitor. This is a documentation issue that doesn't affect functionality but should be fixed for clarity.Important Files Changed
fieldsproperty toEMECoefficientMonitorwith correct storage size calculation logicSequence Diagram
sequenceDiagram participant User participant EMESimulation participant EMECoefficientMonitor participant storage_size participant validate_pre_upload User->>EMESimulation: Create simulation with store_coeffs=True EMESimulation->>EMECoefficientMonitor: Create coeffs_full_monitor Note over EMECoefficientMonitor: fields=("A", "B") by default<br/>num_sweep=None (all sweeps) User->>EMECoefficientMonitor: Optionally set custom fields Note over EMECoefficientMonitor: fields can include:<br/>"A", "B", "n_complex",<br/>"flux", "interface_smatrices",<br/>"overlaps" User->>EMESimulation: Call validate_pre_upload() EMESimulation->>EMESimulation: Build _monitors_full Note over EMESimulation: Includes coeffs_full_monitor<br/>if store_coeffs=True EMESimulation->>storage_size: Calculate storage for each monitor storage_size->>EMECoefficientMonitor: Calculate bytes based on fields Note over EMECoefficientMonitor: Adds bytes for each field:<br/>- A/B: 2*cells*modes²<br/>- n_complex/flux: cells*modes<br/>- interface_smatrices: 4*(cells-1)*modes²<br/>- overlaps: cells*modes² + 2*(cells-1)*modes² EMECoefficientMonitor-->>storage_size: Return total bytes storage_size-->>validate_pre_upload: Return size validate_pre_upload->>validate_pre_upload: Check if size > WARN_COEFF_DATA_SIZE_GB alt Size exceeds warning threshold validate_pre_upload->>User: Warn: coeffs storage too large end validate_pre_upload-->>User: Validation complete