Conversation
…stead of take them as explicit arguments everywhere
…er jsnums changes)
…hadowed the one we're trying to use.
…n array of values as second argument
…ther x-axis label, to help prevent overlaps The approach is awkward, and should be reverted once vega/vega#4238 is implemented. Until then, this approach: Starts in `prepareAxisForOffsets`, which * Creates an empty data table named `xAxisLabelOffsets`, to be filled in later * Creates an ordinal scale named `xAxisYOffsets`, derived from the `xAxisLabelOffsets` table * Adds an encoding step to the xAxis that - names the labels `xAxisLabels` (so they appear as a data set via the view API) - updates the dy offset based on the xAxisYOffset scale * We render the graph once, and then in the continuation, we call `rebuildAxisLabels`, in which we - extract the `xAxisLabels` dataset, - extract their `index` property -- which is a fraction between 0--1, rather than an integer - compute a discrete lookup table from index to (their position % 2) * 25 - shove that lookup table back int othe empty `xAxisLabelOffsets` table - rerender the graph The preparation step sets up the necessary dataflow; the first render lets Vega compute which labels are going to exist; then the rebuilding step computes the desired offsets and causes the dataflow to propagate that information back through.
…not really needed), so that setting the signal doesn't cause a runtime error also, removing all mention of zindex, to avoid a weird hovering bug in chrome-based browsers where the tooltips don't disappear.
This is a stopgap implementation to permit stagger-offsetting every other x-axis label, to help prevent overlaps
…explicitly ask for 0-255 ranges for rgb and a Relevant for #1862
Expose every non-class top-level Numbers function at the module level, each wrapped with a parameterize-style save/setErrbacks/restore so callers can optionally supply their own errbacks as a trailing argument without having to instantiate their own library. Co-Authored-By: Ben Lerner <benjamin.lerner@gmail.com> Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Joe says: Runs locally; testing a push to see if this gets rid of an annoying error-but-not-really-blocking annoyance in the CI Claude says: Jest's open-handle report was flagging a PIPEWRAP from the http-server child: in CI (no TTY, container), cp.spawn's default piped stdio stays open on Jest's side, and npx's grandchild-killing doesn't reliably propagate SIGTERM. - Pass stdio: "ignore" so no pipes are kept alive. - Invoke the http-server script through the current Node process (process.execPath + require.resolve) instead of via npx, so kill targets a direct child and we don't depend on the bin shebang picking the right node on a given CI image. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Alternate design to implementing & testing jsnums
- Fix a few mistakes in charts.arr where the old rawarray-based methods were not working - Clean up redundant "type-checking" iterations in charts.arr, since the annotations are doing that in JS - Simplify image-num-dot-chart-from-list to simply compose image-dot-chart-from-list and the image-labels method - Same for labeled-num-dot-chart-from-list - Add image-dot-chart-from-list, and rewrote categoricalDotChart to handle image marks accordingly. Various examples: render-chart(from-list.dot-chart(species)).display() render-chart(from-list.dot-chart(species).labels(names)).display() render-chart(from-list.dot-chart(species).image-labels(images)).display() render-chart(from-list.dot-chart(species).labels(names).image-labels(images)).display() render-chart(from-list.dot-chart(fixeds.map(to-string)).labels(names).image-labels(images)).display() render-chart(from-list.image-dot-chart(images, species)).display() render-chart(from-list.image-dot-chart(images, species).labels(names)).display() render-chart(from-list.num-dot-chart(ages)).display() render-chart(from-list.num-dot-chart(ages).labels(names)).display() render-chart(from-list.num-dot-chart(ages).labels(names).image-labels(images)).display() render-chart(from-list.labeled-num-dot-chart(names, ages)).display() render-chart(from-list.labeled-num-dot-chart(names, ages).image-labels(images)).display() render-chart(from-list.image-num-dot-chart(images, ages)).display() render-chart(from-list.image-num-dot-chart(images, ages).labels(names)).display() render-chart(from-list.image-num-dot-chart(images, ages).point-size(40).use-image-sizes(false).labels(names)).display()
Joe says: I think this is a regression? The composition makes it so this test has a surprising result IMO. How much is the user supposed to think about sort orderings? Claude says: Adds a regression test that catches mis-pairing between images and categories in image-dot-chart-from-list. The same logical (image, category) bag rendered twice in different input orders should produce identical images, since dot-chart-from-list sorts categories internally and a categorical chart should depend only on the bag, not on input order. Each image is built with text(...) carrying its own category name, so a save-image of a failing run shows exactly which image landed under which column label. Currently fails on this branch: a scrambled-input render rotates each image one slot off its category. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ring
Joe says:
I think this is just a Vega config-format issue (thinko/typo in which kind of
update/signal goes in). I think these images should be different.
Claude says:
The two sizing settings must produce visibly different charts, since
use-image-sizes(true) means "use the image's natural dimensions" and
use-image-sizes(false) means "size them via the chart's dotSize
signal." If both render byte-identically, the false branch is a no-op.
Currently fails on this branch: the ImageMarks width/height encoding
uses {update: "..."} as a value reference, which is not valid Vega
encoding syntax (the standard form is {signal: "..."}), so Vega
silently ignores both width and height and falls back to natural
sizing in either mode. The dotsize/dotSize typo on the height line
is real but masked by this larger problem.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
moves the sorting into the Vega processing, rather than Pyret, so that the lists stay in order throughout Pyret's computation
Add image-dot-chart-from-list
Joe says: I accidentally included a bunch of unreviewed/on-the-wrong-format tests for jsnums in an earlier commit. This (a) reverts back to the minimal jsnums baseline of tests and (b) adds the jsnums tests to the CI (they were missing!) so that we'll notice when we write bad tests here. Claude says: The bulk of tests/jsnums-test/jsnums-test.js (lines 45-1106) was added as part of the closure-errbacks branch merge (eb0d524) without being reviewed on its own merits — the closure-errbacks PR was about a much larger library refactor, and the test-file payload rode along. Running the file under current semantics surfaces a mix of stale-API references (JN._innards.errbacks instead of getErrbacks()), tests asserting old contract behavior, and ~30 calls passing raw JS doubles where pyretnums are expected. Cleaning that up properly is a meaningful project; doing it quietly inside an unreviewed pile is the wrong shape. This change: 1. Reverts tests/jsnums-test/jsnums-test.js to the original 44-line seed from 8f71137, with three small adaptations for closure-errbacks: - Build via JNlib.MakeNumberLibrary(sampleErrbacks) so prototype methods close over this test's throwing errbacks rather than the module's INITIAL_ERRBACKS. - Drop the trailing `sampleErrorBacks` arg from each call — closure-errbacks methods don't take it. - Rename sampleErrorBacks -> sampleErrbacks for consistency. 2. Adds `make jsnums-test` as a CI step so future bad-test-file states actually turn the build red. The previous CI command (`make all-pyret-test`) does not depend on jsnums-test, which is why the broken assertions in the unreviewed bulk slipped past. `make jsnums-test` is now green: 1 spec, 0 failures. Future jsnums coverage should land deliberately, one reviewable spec block at a time, on top of this baseline. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…now) Joe says: Medium-interesting finding from Ben noticing a change to `num-to-fixnum` behavior on CPO. The key thing is NON-integer JS floats. They are *not* supposed to be able to be Pyret numbers. In various places we were lax about this and things worked out because of tostring conversions or === doing the right thing, etc. This commit tries to make some progress on catching our own mistakes here: - Correctly return `false` from the widely used `isPyretNumber` for values like 2.3 (~2.3 aka Roughnum.makeInstance(2.3) still returns true) - Add assertPyretNumber, currently used in one place (toFixnum), which based on its throwing of a DomainError clearly expects to get an actual Pyret number. The rest of the commit cleans up cases where we made mistakes and returned or passed JS floating-point across the boundary. assertPyretNumber *should* throw, not just console.error. But I'm too scared to do that because I'm not convinced our tests are exhaustive here at all, and an exception in the wild could be pretty breaking. So we console.error for now. Claude used this assert on toFixnum to find several errors, all of which come straight from our existing tests/compiler runs. Claude says: Specifics of the boundary cleanups, since they're spread across four files: - js-numbers.js atan2: the y=0 quadrants correctly wrapped Math.PI in a Roughnum, but the x>0 4th-quadrant and x<0 cases passed bare Math.PI and 2*Math.PI to add(), which short-circuits on two JS numbers and returns the raw JS sum. So num-atan2(0, -1) returned the JS double 3.141592653589793 rather than a Roughnum, with branch-dependent type tags visible to user code (num-is-roughnum was false). Wrapping the constants in Roughnum.makeInstance fixes it. Bug present since the original atan2 introduction in 2016. - filesystem-internal.js stat: mtimeMs/ctimeMs from Node fs.Stats carry sub-millisecond precision on filesystems with ns granularity (APFS, ext4-ns, NTFS), so mtime/ctime were leaking fractional doubles to callers like cli-module-loader.arr's cache-staleness check. Floored to integer ms, matching the documented "in epoch ms" contract. - charts-lib.js function-plot: sample x-coordinates were built via raw JS arithmetic (xMin + fraction*i) and handed to the user's Pyret function and to toFixnum, both of which expect pyretnums. Wrapped each sample in jsnums.fromFixnum at the JS/Pyret boundary, in both the initial pass and the recompute callback. Same fix for clippedDomain[0/1] before they get stashed back into globalOptions. - image-lib.js PointPolygonImage: dropped a redundant jsnums.toFixnum(vertices[v].x/y) — unwrapPoint2D already produces fixnums upstream, so the second call was dead under the lax predicate and now (correctly) flags inputs like 2.5 from point(15/6, ...). assertPyretNumber currently console.error's (with a stack) on non-integer JS numbers rather than throwing, per Joe's caution above; clearly non-numeric inputs still throw the same domainError they did before. Pyret-test suite green. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Joe says: Claude helped write some tests for the cases of atan2 that are likely to short-circuit and produce some of the fixnum errors/roughnum-vs-exact-int errors that check-pyret-number was designed for. Builds on #1864, this can rebsae onto horizon once that's merged Claude says: Adds an `it("atan2 four quadrants and boundaries")` block that pins down atan2's return-type contract across every combination of axis sign and quadrant. atan2's x<0 and (x>0, y<0) branches historically leaked bare Math.PI / 2*Math.PI through `add`, because JS-number+JS-number takes a fast path that returns the raw sum unchanged — so atan2(0, -1) returned the JS double 3.141592653589793 rather than a Roughnum, with branch- dependent type tags visible to user code. The library fix landed recently; this spec captures the expected behavior so future regressions trip CI rather than reappearing as branch-by-branch mysteries. Uses `isPyretNumber` rather than `isRoughnum` as the predicate because atan(0) currently short-circuits to an integer fixnum (not a Roughnum), so cases like atan2(0, 1) come back as 0 rather than ~0. That's a separate trig-type-consistency wart and is called out in a comment. Pins atan2(-1, 0) at 3π/2 explicitly: jsnums uses [0, 2π) for the negative-y axis, unlike JS Math.atan2 which would give -π/2 here. If we ever decide to switch conventions, this assertion makes the choice explicit instead of silently regressing user code. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Joe says: Responding to Ben's comments in #1864 Claude says (with Joe edits for brevity): Per Ben's review: recomputePoints now takes an array of JS numbers and produces objects with JS-number x/y. The jsnums.fromFixnum moves to the func.app call site inside recomputePoints, the one spot that needs a Pyret number, since func is user-supplied Pyret code. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Tighten isPyretNumber, fix latent leaks (log toFixnum violations for …
Joe says: Leaving in both isInteger and the toBe(0) because of what Claude said, but also because this test is coming right after a regression/update around isInteger and isPyretNumber, so it's useful to check that Claude says: - atan2(0, x>0) now asserts `isInteger` + `equals(_, 0)`, covering small int, larger int, and bignum x. `equals`/`isInteger` (rather than `===`/`toBe(0)`) keeps the spec at the "exact integer with value 0" level instead of pinning the JS-number representation. - Every other case is provably irrational (multiples of π) and now asserts `isRoughnum` directly. `make jsnums-test` passes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add jsnums-test spec for atan2 quadrant correctness
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.
Recent changes – charts improvements, numbers improvements, dot charts.