fix(plotting): ngspice plotting module - data extraction rewrite + correctness fixes (follow-up to #416)#506
Merged
Eyantra698Sumanto merged 2 commits intoMay 19, 2026
Conversation
…NgspiceWidget This is the follow-up to FOSSEE#416 (esim-modularized-plotting). After the initial merge I kept using the module and found a cascade of bugs that needed fixing before the plotter is actually reliable. Full list with root-cause analysis is in BUGS_AND_FIXES.md in the dev branch. Security: - BUG-006: Replaced raw eval() on user math input with _safe_eval() (AST-based, whitelists numeric literals + arithmetic ops only) Core plotting bugs (plotter was effectively broken on first use): - BUG-005: on_waveform_toggle did nothing — item.isSelected() always False because clicks were absorbed by the child QWidget. Replaced with an explicit toggle; also fixed plt.subplots_adjust() Tk crash. - BUG-019: plot_function accumulated ghost traces — no cleanup of the previous Line2D before replotting. Added _func_line ref + remove(). - BUG-023: Cursor lines vanished on any refresh — fig.clear() wiped axvline objects, cursor_lines held stale refs. Added _restore_cursors() called after every refresh_plot. - BUG-017: Autoscale checkbox was never connected to anything. - BUG-018: zoom_in toggled rubber-band mode instead of zooming; zoom_out navigated back-stack instead of zooming out. Both replaced with proper center-of-axes scaling using DEFAULT_ZOOM_FACTOR. - BUG-020: Escape key couldn't exit zoom/pan — hardwired to clear_cursors(). Now checks nav_toolbar.mode first. Timing diagram bugs: - BUG-007: plot_timing_diagram called .keys() on a List (AttributeError, swallowed by except). Also mutated shared obj_dataext.y in-place. Rewritten to build local y_data dict; obj_dataext never touched. - BUG-009: Right-click Hide and left-click toggle used diverging state. Both now go through t.visible + refresh_plot(). - BUG-010: Single global 70% logic threshold replaced with per-trace midpoint (CMOS switches at VDD/2, not 70%). - BUG-011: threshold_spinbox default was 0V not minimum, so "Auto" mode never activated. Added setValue(minimum()) after setRange. - BUG-012: axhline drawn at raw threshold, not offset by rank*spacing. Replaced single line with per-rank loop. - BUG-013: set_time_axis_label read unsliced obj_dataext.x after transient-start trim. Now accepts optional time_data arg. - BUG-014: Constant DC signal mapped to all-low. Added vmax==vmin guard. - BUG-021: Traces not normalized — voltage-dependent height caused overlap in mixed-voltage circuits. All traces now normalized to {0,1}. - BUG-022: Timing view enabled for AC/DC analysis (meaningless output). Now disabled with tooltip for non-transient simulations. - BUG-024: time_data rebind inside loop corrupted all subsequent traces. Fixed with per-iteration trace_time = time_data[:n]. - BUG-031: _detect_frequency received unsliced time_data instead of per-trace trace_time (contract violation). - BUG-034: timing_annotations retained dead Text refs across view switches. Added .clear() in refresh_plot wipe block. - BUG-036: Autoscale OFF + timing toggle restored normalized ylim into normal view. Added _last_was_timing guard. Style / export / misc: - BUG-025: change_style returned inside loop — only first item updated. Moved refresh decision outside loop. - BUG-026: Local variable named 'format' shadowed Python built-in. Renamed to fmt. - BUG-027: MultimeterWidgetClass had no owner ref — could be GC'd. Added self._meters list; closeEvent cleans up. - BUG-028: open_figure_options called fig.suptitle('') to read title, which also set it to ''. Fixed with fig._suptitle read. - BUG-029: 'vs' substring check matched node names like v(vss). Changed to ' vs ' with spaces + maxsplit=1. - BUG-030: c= kwarg used for plot()/step()/semilogx() — documented only for scatter(). Renamed to color=. - BUG-032: steps-post style rendered as steps-pre — axes.step() default is where='pre'. Added plot_kwargs dict with where='post'. - BUG-035: Three independent time-scale chains, two missing zero-span case. Extracted _get_time_scale_and_unit() as single source of truth. - BUG-016: Right-click opened Qt context menu AND placed cursor 2 simultaneously. Removed canvas context menu (Qt.NoContextMenu); left/right click now map cleanly to C1/C2. Added drag-to-move. - REFACTOR-001: Replaced 6 parallel trace dicts with TraceModel. Trace.update_line() handles live style/color/thickness without replot. New feature: - FEATURE-001: Timing view right-side annotations: VOH/VOL labels (H:/L:) with SI-prefixed values, frequency label for periodic signals using rising-edge CV filter to suppress glitch/one-shot false readings. Constant DC signals show DC: label. Current traces use A/mA/µA/nA. Full rewrite. Old parser read each file 2-3 times with fragile line-index arithmetic and broke on single-column-group circuits (BasicGates: produced 164 pts, should be 166 — pre-existing bug fixed). New: single-pass state-machine (_parse_plot_file), handles multiple column groups + page-break headers, all three analysis types. Public interface unchanged — openFile/computeAxes/numVals call sequence still works; all attributes still exist. Also fixed: BUG-033 — log label order was wrong (AC/DC/Tran indices mapped to wrong strings). Replaced positional split with explicit dict. Verified identical output across 11 Example circuits: Transient (Monostable555 205k pts, Half_Adder, BasicGates), DC (BJT_CE_config nested sweep, FET_Characteristic), AC dec (High_Pass_Filter, Low_Pass_Filter, BJT_Frequency_Response 701 pts), AC lin (Parallel_Resonance), Tran+subcircuit (Integrator_LM_741 27 nodes w/ duplicate names, Precision_Rectifiers_using_LM741 76 nodes). - BUG-008: Dead plotFlag read removed (was never written anywhere). plotFlag2 renamed to redoPlotFlag (also updated in TerminalUi.py). Unquoted shell commands fixed: gaw and xterm use shlex.quote(); mintty switched from single shell string to QProcess.start(prog, args). - BUG-015: MergedChannels collapsed stderr into stdout, making the ngspice noise filter permanently dead. Changed to SeparateChannels with dedicated _handle_stdout / _handle_stderr slots. - REFACTOR-002: _register_process() extracted from duplicated 4-line pattern in _start_process and open_ngspice_plots. Renamed setProperty("plotFlag2") → setProperty("redoPlotFlag") to match NgspiceWidget.py rename. Must move together with NgspiceWidget.
- QListWidget.MultiSelection → SelectionMode.MultiSelection - menu.exec_() → menu.exec() - QProcess.SeparateChannels → ProcessChannelMode.SeparateChannels - error_type <= int comparison (TypeError) → enum tuple membership check - QProcess.NotRunning (×2) → ProcessState.NotRunning in TerminalUi
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fix(plotting): ngspice plotting module - data extraction rewrite + correctness fixes (follow-up to #416)
Follow-up to #416. After that merged we ran the new plotter against all 11 example circuits across all three analysis types and did extended manual testing on real designs. This PR has everything that came out of that. All 5 files are PyQt6 ready and compatible with #497.
Files changed:
plot_window.py,NgspiceWidget.py,data_extraction.py,plotting_widgets.py,TerminalUi.pyWhat is in this PR
Data Extraction - Full Rewrite
This is the main change in the PR. We rewrote the entire data parser from scratch.
The old parser was reading each output file multiple times and using line-index arithmetic to find data. This was working fine for simple circuits but was breaking silently on several ngspice output formats that show up in real projects. We replaced it with a single-pass state machine in
_parse_plot_filethat handles everything in one read.What we fixed in the process:
Multi column group files - ngspice splits wide node tables across multiple sections with
*headers when there are too many nodes. The old parser was only reading the first group and silently dropping the rest.Page break headers - ngspice repeats the
Indexheader row every 55 or so data rows. The old parser was treating each of these as the start of a new data group which was messing up the data.AC complex format - AC analysis rows have two tab columns per node, real and imaginary parts. The real part also has a trailing comma that ngspice puts there. We now strip that and discard the imaginary column, matching the original behaviour.
Duplicate node names - ngspice truncates long node names and two different nodes can end up with the same string. The old dict based approach was merging them into one channel with double the data. Fixed with positional lists so each column gets its own slot.
Pre existing truncation bug - there was a 2 row truncation in the original parser that we also fixed here. BasicGates was showing 164 points, correct count is 166.
Tested against all 11 example circuits - Transient, DC, AC dec and AC lin, subcircuits, and circuits with up to 76 nodes. Parses Monostable555 (205k data points, 22 nodes) in 0.89s. Output verified to be identical to the original parser across all circuits.
The public interface is unchanged so nothing breaks.
openFile(),computeAxes(),numVals()all work exactly as before.Digital Timing View
Tested the timing view on real circuits and found several things that needed fixing.
Threshold logic - updated to per-trace midpoint (VDD/2). This is how every scope and logic analyzer auto thresholds and it works correctly on mixed voltage circuits. Manual spinbox override still works.
Trace height - updated to use uniform normalised heights regardless of voltage domain. On a mixed voltage circuit like 3.3V and 5V signals together the traces were overlapping because height was proportional to actual voltage swing.
Threshold lines - fixed the threshold line position. Each trace is stacked at rank times spacing in y-space and the line was not accounting for that so it was in the wrong position for every trace except the bottom one.
Disabled for AC and DC - the timing view was enabled for all analysis types. AC data is frequency domain so showing it as a digital timing diagram gives a meaningless plot. Disabled it with a tooltip for non-transient analysis and added a guard inside the function as a second layer.
Constant DC signals - when a signal had zero swing the threshold comparison always came out False and showed the signal as all-low regardless of actual voltage. Now detected separately and shown as
DC: X.XX V.Transient offset handling - fixed the transient start time offset. It was trying to slice
obj_dataext.yin-place but calling.keys()on a Python list which throws an AttributeError. The exception was being swallowed silently so nothing was working. We now build a localy_datadict per render and never touchobj_dataext.Per trace time slice - fixed a variable leak in the trace loop. When one trace had a shorter y-array than the time axis the code was trimming the outer
time_datavariable which was then affecting all the traces after it. Now uses a localtrace_timeper iteration.Timing to normal view switch - fixed an issue where switching from timing view back to normal view with autoscale off was restoring the timing y-limits into a voltage plot. Signals spanning 0-5V were showing as a flat line near the x-axis. Added a
_last_was_timingflag to block this.Annotations - each trace now shows VOH and VOL from the actual simulation data, for example
H: 4.97 VandL: 0.02 V. Periodic signals also show detected frequency like1.00 MHz. Constant DC signals showDC: 3.30 V. Current traces use proper units likemAanduA. Frequency detection uses rising-edge timing with a guard to filter out non-periodic signals.Cursor System
Right click conflict - right click was triggering both a Qt context menu and placing cursor 2 at the same time. The context menu items were already in the toolbar and side panel so we removed it from the canvas entirely. Left click is C1, right click is C2, no conflict.
Drag to reposition - added drag support.
_find_nearest_cursor()checks if the click is within 8px of an existing cursor. If yes, dragging moves it in-place instead of dropping a new cursor on every click.Cursors disappearing after redraw -
refresh_plotcallsfig.clear()which wipes all matplotlib artists including the cursor lines. The positions were stored but the lines were never recreated after a redraw so toggling a trace or changing the grid was making cursors vanish visually. Added_restore_cursors()to recreate them after every refresh.Unified time scale - the time unit selection logic was copy pasted in three places and two of them had a bug with zero-span simulations. Pulled it into one method
_get_time_scale_and_unit()that everyone now calls.General Plotter
Waveform clicks not registering -
item.isSelected()always returns False when list items have custom child widgets viasetItemWidget()because clicks land on the child widget and Qt never updates the selection model. Replaced with an explicitt.visibletoggle.Hide and left-click out of sync - right click Hide was calling
line_object.set_visible(False)directly without updatingt.visible. Any redraw after that would re-readt.visible = Trueand silently bring the trace back. Both paths now go throught.visibleandrefresh_plot().Plot Function stacking traces - clicking Plot Function multiple times was stacking traces on top of each other with no way to clear. We now remove the previous result before adding a new one, tracked in
self._func_line.eval() on user input - the expression input was going directly to Python's
eval(). Replaced with_safe_eval(), an AST based evaluator that only allows trace names and arithmetic operators.vs matching node names -
if 'vs' in function_textwas matching any node name that contains vs likev(vss),avss,vsupplyand so on. These are very common supply rail names in circuit simulation. Changed to check forvswith spaces on both sides.steps-post rendering as steps-pre -
axes.step()was being called without awhereargument which defaults to'pre'. Transitions were drawing one sample too early. Now passeswhere='post'.Multimeter widget getting garbage collected -
MultimeterWidgetClasshad no parent and no persistent reference outside the loop it was created in. Python could GC it and close the window right after creation. Now kept inself._metersfor the lifetime of the plot window.Figure Options wiping the title -
self.fig.suptitle('').get_text()was being used to read the current title. Callingsuptitle()always writes so the title was being cleared every time the dialog opened. Fixed by readingself.fig._suptitledirectly.Autoscale checkbox doing nothing -
stateChangedwas never connected andrefresh_plotwas never reading the checkbox state. Now properly wired up. When autoscale is off, zoom limits are preserved across redraws.Zoom In and Out menu actions - Zoom In was toggling the toolbar rubber band selector mode. Zoom Out was going back in the view history. Neither was actually zooming. Both now do a proper scale step from the centre of the current view.
Escape key stuck in zoom or pan mode - Escape was always calling
clear_cursors()regardless of what toolbar mode was active. Now checks the toolbar mode first and exits zoom or pan if active.format shadowing Python built-in - a local variable named
formatwas shadowing the built-informat()function in that scope. Renamed tofmt.Simulation Console
Stderr filter was never running -
QProcess.MergedChannelsmerges stderr into stdout at the OS level soreadAllStandardError()always returned empty bytes. The noise filter for NGSpice batch mode messages was dead code from the beginning. Changed toSeparateChannelswith separate slots for stdout and stderr so the filter actually runs now.Shell commands breaking on project paths with spaces - GAW and xterm commands were built with plain f-strings so any project name or path with a space was breaking the shell command without any error. All paths now go through
shlex.quote(). Windows mintty was also switched toQProcess.start(program, args)so Qt handles quoting.Dead property read removed -
finish_simulationwas reading a"plotFlag"property from the process that nothing in the codebase was ever writing. Removed. Also renamed"plotFlag2"to"redoPlotFlag"in both files for clarity.The complete root cause analysis and notes for each fix are in
BUGS_AND_FIXES.mdin the repo root for anyone who wants to dig deeper.