Fix/nomiss qualifier - #29
Merged
Merged
Conversation
'bar dN from dM ...' is GLE's stacked-bar form -- dN draws on top of dM.
_parse_bar_command tokenizes it to two dataset-shaped words with no
'from'-awareness, so it already fell into the existing "no BarSeries
model for a shared bar group" raw-passthrough path (Finding 17's grouped
'bar d1,d2 fill c1,c2' form). But an EARLIER, independent 'bar dM fill
...' statement for the same dataset had, by then, already been modeled
as its own BarSeries: its regenerated 'data' line only defines dM, so the
still-raw 'bar dN from dM ...' line was left referencing a dN nothing
defined. GLE rejected the round-tripped script outright ("bar dataset dN
not defined") -- a compile failure, not a cosmetic difference, on any
manual-style bar chart using GLE's stacked-bar syntax (GLEstudio's S8
rendered-fidelity corpus: graph/fig/gc_bargraph1.gle).
A tempting narrower fix -- synthesize a second 'data f.dat dN=cX,cY' line
just for the orphaned dataset -- turns out to be unsound: GLEWriter always
rewrites an owned series' data file with exactly the columns that series
uses (add_bar_chart/_write_columns), so the sibling's modeled BarSeries
would silently truncate the physical file down to its own columns,
leaving the synthesized reference pointing past the end of a file gleplot
itself just shrank. Caught empirically while diagnosing this: a naive
version of that fix only "worked" because a manual copy-back of the
pristine data file was masking the truncation in ad hoc testing.
Fix instead in pass 1 (_parse_graph_block): scan the whole graph block for
'bar dN from dM ...' up front and remember both names in the new
'_bar_stack_datasets' set, before pass 2 dispatches anything. When pass 2
later reaches an independent, single-name 'bar dM fill ...' for a name in
that set, it now stays raw too. Both sides of the stack end up
unconsumed, the existing 'data'-statement reconciliation restores the
whole original line verbatim, and gleplot never touches the underlying
data file at all -- GLE reads its real, complete bytes at compile time.
Finding 20 test battery: the stacked pair kept raw together (body order,
warnings, no stale "may not resolve" note, unmodified data file on
save), independent non-stacked bars from the same file unaffected, the
single-dataset case byte-identical, a save->parse->save fixed-point
regression, and a gle-marked end-to-end test compiling the original and
round-tripped reproducer with real GLE and asserting pixel-identical
renders. Manually verified against the actual GLE-manual reproduction
(graph/fig/gc_bargraph1.gle): compiles clean and renders pixel-identical
to the original.
Fixed-point battery green (exemption set empty), full suite green
(2565 passed / 2 skipped, +5 over baseline), gle-marked suite green,
black clean on the added lines, no new flake8 or mypy findings.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… None Two bugs conspired to turn 'd1 lstyle 2 color red' (the GLE manual's own 'nomiss' example -- no 'line' keyword at all) into a compile failure on round trip: GLE reports "invalid marker name 'None'" (GLEstudio's S8 rendered-fidelity corpus: graph/fig/gc_nomiss.gle). Root cause, in the recognizer (_scan_series_attrs): GLE draws a line for a 'dN' display command when EITHER the 'line' keyword is given OR 'lstyle' names a style at all -- graph2.cpp's own draw-a-line test is 'dp[dn]->line || dp[dn]->lstyle[0] != 0'. has_line only ever became True for the literal 'line' keyword, so a bare 'lstyle' recovered as linestyle="none" with no marker. That fed a marker=None, linestyle="none" series into a real, independent writer bug: GLEWriter.add_plot_line's no-line branch interpolated `marker` into an f-string with no `if marker:` guard, unlike its own line+marker branch and every other marker-emitting site in the writer (add_errorbar, add_errorbar_from_file). The literal text "marker None" reached the script. This branch is also reachable directly from the scripting API (ax.plot(x, y, linestyle='none') with no marker -- a real, if pointless, degenerate case matplotlib itself allows), independent of any GLE import, confirming the writer needed its own fix, not only the recognizer's. Fixed both: 'lstyle' now sets has_line, matching GLE's own semantics; and add_plot_line's no-line branch only emits a marker clause `elif marker:`, emitting nothing at all when a series has neither a line nor a marker (there is nothing to draw). Audited every other marker-emitting site for the same class of bug per the reviewer's instruction. Found and fixed a second, related drop while building the repro: 'd2 nomiss lstyle 1 marker diamond ...', when recovered as a reference-mode FileSeries (a hand-written file with no gleplot metadata block), silently lost its marker entirely on save -- GLEWriter.add_plot_line_from_file never had a marker/markersize parameter at all, and Figure._render_axes never passed one through. This is not a "None leak" (no literal "None" reached the script) but the same family of gap in the writer's dN sub-command emission the reviewer's diagnosis named, and it was necessary to get pixel parity on the actual manual figure's d2 curve, not merely a compiling script. color/lstyle sites elsewhere in the writer were already correctly guarded (`if color is not None`, no unguarded Optional lstyle interpolation anywhere) -- marker was the only leaking field. Finding 21 test battery: bare-lstyle-is-a-line at the object-model level, the manual's exact nomiss shape with no "None" anywhere in warnings or output, a dedicated writer-level regression for the scripting-API degenerate case (independent of any .gle file), a save->parse->save fixed-point regression, and a gle-marked end-to-end test compiling the original and round-tripped reproducer with real GLE and asserting pixel-identical renders (explicit 'set hei' and 'lwidth' pin gleplot's own unrelated defaults so they cannot mask a real difference). Manually verified against the actual GLE-manual reproduction (graph/fig/gc_nomiss.gle): both the original and the round-tripped script now compile cleanly (previously the round trip failed outright). Not fixed here, out of scope: the manual figure's own 'nomiss' qualifier (ignore missing values so a gapped line draws unbroken) is itself silently dropped -- unmodeled anywhere in the recognizer, on d1 or d2 alike -- so the real gc_nomiss.gle's d2 curve still regains a gap at its missing value after round-trip, and the figure's overall placement also shifts slightly (gleplot's auto-margins vs. the manual's own manual_graph_mode macro). Both compile cleanly either way; flagged as a separate, pre-existing finding rather than folded into this fix. Fixed-point battery green (exemption set empty), full suite green (2570 passed / 2 skipped, +5 over prior commit), gle-marked suite green, black clean on the added lines, no new flake8 or mypy findings. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
_scan_series_attrs had no branch for 'nomiss' at all -- the token loop's generic fallthrough silently dropped it, with no field on the object model and no round-trip, even though the writer's own 'marker None' fix (Finding 21) already used the manual's gc_nomiss.gle as a reproducer. That fixture's data has no actual missing value, so it never exercised the keyword itself: both the original and a round-tripped script compile fine either way, so nothing but a rendered-pixel comparison against real missing data catches the drop. Adds a `nomiss: bool` field to LineSeries/ScatterSeries and the FileSeries 'line' variant, recognizes the keyword in _scan_series_attrs, and re-emits it from GLEWriter.add_plot_line/add_plot_line_from_file. Proven against the GLE manual's own tut3.dat missing-value row with a real GLE compile + pixel-parity round trip. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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.
_scan_series_attrs now recognizes nomiss, threads it onto LineSeries/ScatterSeries/FileSeries, and GLEWriter re-emits it. Proven with a real GLE compile + pixel-parity round trip against the manual's own tut3.dat missing-value row (confirmed the new test actually catches the regression by temporarily reverting the writer fix and watching it fail, then re-verified green). Full suite (2573 tests), the gle-marked integration battery (26 tests, including the two new ones), and black were all clean; flake8/mypy have pre-existing environment issues unrelated to this change (no line-length config matching black's 88, and a mypy/numpy-typeshed incompatibility) — confirmed identical on the unmodified baseline.