Conversation
There was a problem hiding this comment.
Pull request overview
This PR makes small style/behavior adjustments across the simulation modules and scripts, including updates to test formatting, a change to GR801 fault injection scaling, UTC timestamp generation updates, and an FFT Nyquist-handling change in the toy simulation pipeline.
Changes:
- Reformats GR801 simulation tests and config dict literals (quotes/whitespace).
- Updates
simulation_pipeline_gr801.inject_faults()chip area scaling to make faults more likely in short tests. - Adjusts FFT wavenumber grid generation to zero Nyquist modes (and updates scripts to use timezone-aware UTC timestamps).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_simulation_pipeline_gr801.py |
Test formatting updates; still contains probabilistic behavior that can be flaky without seeding. |
simulation_pipeline_gr801.py |
Style tweaks + changes fault injection scaling; still uses print() for logging in changed regions. |
simulation_pipeline.py |
Alters _k_grids() Nyquist handling; currently impacts k2 used beyond first-derivative operators. |
scripts/run_simulation.py |
Uses timezone-aware UTC timestamp for run IDs. |
scripts/anatel_api_diagnose.py |
Uses timezone-aware UTC timestamp in generated report. |
Comments suppressed due to low confidence (1)
simulation_pipeline_gr801.py:178
safety_violation_detectedemits aprint()on threshold breach; this makes library use noisy and hard to test. PreferLOGGER.warning(...)(or return the condition and let the caller decide how to report) to follow the repo’s logging convention.
'performance': state.soc.performance,
'total_faults_injected': state.faults_injected,
'total_faults_corrected': state.faults_corrected,
'application_accuracy': state.app.accuracy,
}
return metrics
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_inject_faults(): | ||
| """Test fault injection mechanism with deterministic seeding.""" | ||
| # Seed the random number generator for reproducibility | ||
| np.random.seed(42) | ||
|
|
||
| soc = sim.SoC(num_cores=4, memory_size=1024, accelerator_present=True) | ||
| radiation = sim.RadiationModel(particle_flux=10.0, upset_rate=0.1) | ||
|
|
||
| radiation = sim.RadiationModel(particle_flux=100.0, upset_rate=0.5) | ||
|
|
||
| # Run multiple times to ensure some faults are injected | ||
| # With high flux and upset rate, we should consistently see faults | ||
| total_faults = 0 | ||
| for _ in range(10): |
There was a problem hiding this comment.
test_inject_faults is probabilistic (asserts total_faults > 0) but the test suite does not seed NumPy’s RNG, so it can be flaky (rarely producing 0 faults). Seed deterministically (e.g., np.random.seed(...)) or switch to a local np.random.default_rng(...) generator so CI runs are stable and reproducible.
| "faults_injected": state.faults_injected, | ||
| "faults_corrected": state.faults_corrected, | ||
| "application_accuracy": state.app.accuracy, | ||
| 'time': state.time, |
There was a problem hiding this comment.
This module uses print() for runtime logging. Elsewhere in the repo (e.g., simulation_pipeline.py) logging goes through a module-level LOGGER = logging.getLogger(__name__). Please switch log_state to use a logger instead of print() so output can be controlled via log levels/handlers.
| LOGGER.info( | ||
| "Time: %.2fs, Errors: %d, Performance: %.2f", | ||
| metrics['time'], metrics['errors'], metrics['performance'] | ||
| ) |
There was a problem hiding this comment.
trigger_safe_shutdown prints directly. Prefer routing this through the module logger (and/or returning a structured shutdown reason) to keep CLI output and library behavior separate and consistent with the rest of the codebase.
|
|
||
| # For even-sized real grids, the Nyquist mode is self-conjugate and must have | ||
| # a purely real FFT coefficient. Multiplying it by i*k to form a first | ||
| # derivative would yield a purely imaginary coefficient, which cannot | ||
| # correspond to a real-valued field on the grid. The discrete sampled | ||
| # derivative of the Nyquist cosine mode is identically zero at grid points, | ||
| # so we zero the Nyquist wavenumber for *first-derivative* operators. | ||
| if nx % 2 == 0: | ||
| kx_1d[nx // 2] = 0.0 | ||
| if ny % 2 == 0: | ||
| ky_1d[ny // 2] = 0.0 | ||
| if nz % 2 == 0: |
There was a problem hiding this comment.
In _k_grids, zeroing the Nyquist entries of kx_1d/ky_1d/kz_1d changes k2 as well, but k2 is used for non-first-derivative operators (e.g., propagate_wavefunction’s kinetic term and Gauss constraint projection). This makes the Laplacian/Poisson operator incorrect for Nyquist modes on even grids. Consider leaving the raw FFT frequencies intact here and instead handling the Nyquist special-case only inside the specific first-derivative routines (e.g., _divergence_spectral), or add a separate helper/flag so callers can request “first-derivative-safe” k-grids without affecting k2.
Description
Impact
User Benefit
Business Value
Roadmap Alignment
Type of Change
Testing
pytest tests/ -v)Checklist
Related Issues
Closes #
Screenshots (if applicable)
Additional Context