[WIP] Try out the new math-expressions - #1622
Open
siefkenj wants to merge 28 commits into
Open
Conversation
Member
|
Right now, merges to main auto-publish to the I'll have to learn the best ways to maintain multiple lines. If you have suggestions, let me know! |
Member
|
Or maybe, we release one more |
# Conflicts: # packages/doenetml-worker-javascript/package.json
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.
This PR is a first step in using the rust-based math-expressions. Currently some tests will fail. There are two levels of integration.
Level 1: use math-expressions compat layer as a drop-in replacement. This will negatively impact build sizes, but should minimize code changes.
Level 2: Pull in the math-expressions directly as a rust dependency and expose via core. This would enable the Rust core to directly call math expressions, as well as eliminate a second WASM bundle.
This is a Level 1 trial to get some of the bugs ironed out. It currently has a pinned submodule, but when we know what we need, this should change to a published package/crate.
Below is an AI summary of some of the issues that need to be resolved
[WIP] Switch to the Rust/WASM math-expressions engine
Replaces the legacy
math-expressions@2.xJavaScript library with the Rust core compiled to WASM,via the
math-expressions-js-compatdrop-in. This branch switches permanently — Rust is thedefault and the only supported configuration.
Suite: 3469 tests, 2965 passed, 465 failed (85.5%), up from 969 failures (70.9%) on the previous
submodule pin.
How the numbers were verified
Every step was checked test-by-test against the previous run —
fixed / broken / gone / new—not by comparing totals. A change that fixes 70 tests and breaks 3 looks like "+67" in a summary and
like a bug in this output. Both DoenetML-side changes came in at 0 broken.
8ccd98d(previous)cdc5343, nothing else changedfromAst(Expression)unwrap restoredTwo new tools do the checking, both added here:
scripts/compare-test-runs.py --before <a>... --after <b>...— diffs two Vitest runs by<spec> :: <test title>, printing regressions in full.scripts/analyze-math-divergences.py <report>...— buckets failures by the mathematics thatdiffers (ordering, exact-vs-decimal, notation, precision) rather than by which spec they live in.
The full suite exhausts the heap in one process, so runs are sharded (
vitest --shard=i/8). This isa pre-existing DoenetML memory problem, not an engine difference — the JavaScript engine OOMs
identically.
What changed
Engine selection
packages/math/src/engine.tsnow re-exportsengine-rust.DOENET_MATH_ENGINE=jsrebuilds againstthe legacy library — kept deliberately, not as a supported mode but as a differential-debugging
tool: when a spec disagrees, the first question is always "does this pass on the old one?", and
rebuilding one package to answer it beats bisecting a behavioral difference by hand.
Building now requires a Rust toolchain (
wasm32-unknown-unknown+ a matchingwasm-bindgen-cli).engine-rust.tsis nearly a straight re-exportIt previously carried three local gap fills. Upstream absorbed all three, and each was deleted
here to verify the upstream fix actually covered our usage — that deletion is the point of the
seam. A local patch that cannot be removed is a fix that did not land.
Gone:
Expression#f(), the context-level operation family (me.simplify(expr)alongsideexpr.simplify()), and the replacer that keptfromAstfrom losingNaN/±InfinitytoJSON.stringify.One fill remains — see below.
WASM loading no longer needs a bundler rule
Upstream added a
setWasmModuleinjection point, replacing the Vite plugin that used to aliascompat's node-only
lib/_wasm.tsto our loader. A build-tool rule no longer stands between thelibrary and its own loader.
One ordering constraint, documented in both files it affects. Compat's
Contextobject literalcontains
_assumptionsHandle: new wasm.Assumptions(), which touches the WASM while the barrel'smodule body is evaluating. So importing
setWasmModulefrom the package root forces that body torun first and the injection always loses to compat's node fallback — silently. We import from
lib/_wasm(a leaf that imports nothing but a type) and orderengine-rust.ts's imports so theloader is evaluated first.
dopricrosses the seamLegacy exposed numeric.js's integrator as
me.math.dopri; the Rust engine has nome.mathentryfor it and provides its own
solve_ode-backed equivalent. Both engines now exportdoprifrom@doenet/mathunder one name, soODESystem.jsandpackages/utils/src/components/function.tsstay engine-agnostic.
me.mathis otherwise complete on both engines.Test expectations: unpadded container delimiters
316 string literals across 27 files, via
scripts/unpad-container-delimiters.py:Two traps worth recording for anyone doing this elsewhere:
toLatexis byte-identical on both engines —\left( 1, 2 \right)keeps its padding. Onlythe text renderer changed. A naive whitespace sweep over test files corrupts LaTeX assertions.
The codemod skips any literal containing a backslash.
collapsed
"...from the group: (, )."— literal prose parentheses around two blank references —and broke a test. The tightened rule also correctly preserves partially-blank tuples like
(2, )and a deliberatea^{ }.The codemod is scoped to quoted-string contents; template literals are excluded because they carry
multi-line DoenetML source where the "padding" is indentation.
The one fix worth reviewing carefully
fromAstmust unwrap anExpression— 95 hard errors, the single largest fix in this PR, andnot a spacing issue.
Legacy accepted an
Expressionanywhere a tree was expected:Compat serializes the object as-is, so it reaches Rust as a bare JSON object with no
$key.The error message is actively misleading. It reads
unknown special None— but thatNoneisthe
Option<&str>fromvalue.get("$").and_then(Value::as_str), not the{"$":"None"}specialupstream just added.
{"$":"None"}works correctly. Worth knowing before anyone else chases it.I assumed this was test sloppiness and checked before fixing it. It isn't: our source depends on
it in
PiecewiseFunction.js,StateVariableEvaluator.tsandDependency.ts. A math-valued statevariable holds an
Expression, and code that re-wraps one hands it straight back tofromAst.With ~675
fromAstcall sites, finding them all by inspection is not realistic.Restored in
engine-rust.ts, recursively (anExpressioncan also sit inside a tree underconstruction, e.g.
["+", someExpr, 2]). Filed upstream as §1 —astReplaceralready visits everynode during
JSON.stringify, so it can do this for free where ours costs a second traversal.What still fails, and whose it is
Upstream (filed in
MATH_EXPRESSIONS_UPSTREAM_REQUESTS.md)simplify()folds no numeric-function applications.floor(55.33),ceil(2.1),abs(-3),sum,prod,mean,variance,std,count,max,min,median,logall stay as["apply", …]. Legacy folded every one. Noteevaluate_to_constant()handlesfloor/ceil/abs/logidentically on both engines — the capability exists, it just isn't reached fromsimplify. Student-visible:<math simplify>sum(3,17,5-4)</math>renders the unevaluated application.0/0simplifies to0; must beNaN.1/0→Infis correct on both. This is how DoenetML computes an undefined slope, so a degenerate line reports slope0— a wrong number rather than a visible failure, which is the worst shape for a grading path.evaluate_numbers({skip_ordering:true})throws. Backssimplify="numberspreserveorder". Throwing beats silently reordering (1+x+2→x+3was the bug it replaced), but the feature has no implementation.log_b(b^n),nCr/nPr/binom, inverse trig at exact points. All pass numerically; onlysimplifyOnComparefails. Worth noting the Rust parse is better: legacy readsin^(-1)(1)as(1/sin)(1), i.e. reciprocal rather than inverse.Decisions to settle, not bugs
(
5.252*10^(-13)vs0.0000000000005252). DoenetML has anavoidScientificNotationattribute,which presumes scientific is the default — so we can't simply absorb this. Render option, or our
display layer?
5/2renders2.5;1/3correctly stays\frac{1}{3}. Ours perupstream's notes. Caveat: if the structural criteria (
ReducedFraction,ExactValue, …) are meantto be usable after
simplify, this stops being a display question.Ours
.treereturns{"$":"Inf"}/{"$":"NaN"}where legacygave JS
Infinity/NaN, and the text printer spells a blank_where legacy printedNaN.Upstream's symmetry argument is sound; our
.treeconsumers dotypeof x === "number"and that'sours to fix.
fromAst(9).Point.jsbuildsArray(n+1)and fills only thecomponents being set;
JSON.stringifyturns the holes intonull, correctly rejected withunexpected value null. Left deliberately unfixed — a hole means "no desired value for thiscomponent", and mapping it to
{"$":"None"}is a semantic guess that should be a human call..eq(); they now differ by 2 ULP (13.445069170765525vs...523). AcloseTofix, but out ofscope for the one test change that was authorized here.
default_orderno-op.Sizes
web-target WASM is 1.32 MB (beforewasm-opt, unavailable in this container).dist/engine-rust.jsis 3.78 MB raw / 1.21 MB gzipped with the WASM inlined, against ~1.1 MBfor the JavaScript library it replaces. Bundle size is not the obstacle the plan feared.
Inlining as base64 mirrors what
CoreWorker.tsdoes forlib_doenetml_worker_bg.wasm: itinstantiates from bytes and needs no
fetch, which matters becausefetchis blocked forblob/data URLs in the VS Code web-worker extension host (#1375).
Not done
interner_size()gives us the gauge; growth rate unmeasuredinitSyncin a real browser Web WorkerFiles worth reviewing first
packages/math/src/engine-rust.ts— thefromAstunwrap and why it's therepackages/math/src/wasm-loader.ts— the injection and the ordering constraintscripts/unpad-container-delimiters.py— the LaTeX and prose-parenthesis carve-outsMATH_EXPRESSIONS_UPSTREAM_REQUESTS.md— the four upstream asks, with reproductions