Skip to content

add scale keyword - #91

Merged
ChrisRackauckas merged 2 commits into
SciML:mainfrom
ranocha:hr/scale_keyword
Nov 15, 2026
Merged

ChrisRackauckas merged 2 commits into
SciML:mainfrom
ranocha:hr/scale_keyword

Conversation

@ranocha

@ranocha ranocha commented Nov 8, 2026

Copy link
Copy Markdown
Member

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

Additional context

I ran into some problems where setting scale = 0 in KLU helps to get better results.

@ChrisRackauckas

Copy link
Copy Markdown
Member

🤖 Automated comment from an AI agent running as @ChrisRackauckas — not written or reviewed by Chris.

PR: #91

Reviewed head: a53d13e
Raw SHA-256: aec19a21b3e3c4807c8bd505249b1fbabcf2f74ce87d7f5aa9220c25513801c3
Source SHA-256: a00a5107c5391bc9184928dea6614deba1b81ff33452b71b899ee44263ccdfc1
Source: standard-review-v1 (harness-log)
Reviewer session: 01a1d25d-5355-7871-962c-f643f3edc088
Slot: 1
Confidence: unknown
Own risk fixed before author rating/discussion: yes

Risk assessment

  • Risk: low (L8)
  • Verdict: CHANGES
  • Reviewed head: a53d13e

Review report

VERDICT: CHANGES
Round: 1
Head: a53d13e
Slot: 1
CI: formatter or lint is not green
Session: 01a1d25d-5355-7871-962c-f643f3edc088

Risk

low (L8: documented optional scale keyword on existing entry points; default scaling remains 2).

Blocking findings

  • [P2] test/test_scale.jl:22 — Confirmed by running: Runic exits 1 on the new low-level-constructor assertion at lines 22–23. It requires the multiline call layout with the opening parenthesis, arguments, and closing parenthesis on separate lines. Format this test with the repository's Runic formatter and rerun the check.

  • [P2] src/PureKLU.jl:561 — Needs verification: the affected Core suite and targeted numerical probes could not execute. The resource admission command waited one hour and exited 75; the required available-memory and test-worker conditions never held together. Its final reported wait included MemAvailable=16837040 KiB; Julia root jobs=5; load=20.56, leaving no slot for another test worker. Run Core and the prepared probes before treating the scaling paths as verified. This finding records an environment limitation; no numerical failure was observed. Missing runtime evidence remains a verification gate, not an ordinary deferred P2 code issue.

  • Verdict: CHANGES

  • Round: 1

  • Blocking priorities: P2, P2

  • Deferred P2 issues: none supplied

Non-blocking findings

None. From inspection, both entry points forward the keyword correctly, the default matches KLUCommon's existing value, and the added pivot/scaling expectations follow from the input matrix. No dependency additions or removed/weakened assertions appear in the diff. These are inspection findings, not numerical execution results.

What I ran

AI reviewer: codex-cli 0.164.0, model gpt-6.1-sol; local session identified above (no session URL available).

  • git diff origin/main...HEAD, surrounding implementation/test inspection, and final git rev-parse HEAD: matched the requested head. Root AGENTS.md and CLAUDE.md were absent.
  • julia --version: julia version 1.13.2.
  • uv run --locked python --version: Python 3.15.0, with warning that --locked has no effect outside a project. No uv.lock was found; this supplies no Python project-lock verification.
  • julia --startup-file=no ../scratch/prepare.jl: exit 0. Created a scratch environment from the repository's dependencies, test extras and compat, developing an exact-head scratch snapshot. Resolved packages included PureKLU 1.7.0, KLU 0.6.1 and PrecompileTools 1.4.0. Preserved the supplied layered JULIA_DEPOT_PATH.
  • julia --startup-file=no --project=@runic -m Runic --check --diff src/Common.jl src/PureKLU.jl test/test_scale.jl: exit 1; only the new assertion's layout differed among these files.
  • typos Project.toml README.md docs/src/index.md src/Common.jl src/PureKLU.jl test/test_scale.jl: exit 0.
  • git diff --check origin/main...HEAD: exit 0.
  • bash ../scratch/run_checks.sh: exit 75 after the one-hour resource wait. No runtime test log was created and no numerical tests executed. The queued command selected GROUP=Core, the scratch test environment, bounds checks, and one Julia/BLAS thread.
  • Final git status --short: empty. All started commands have finished; no checkout edits or GitHub writes.

What I did not verify

  • Core, including the new scale tests; QA; documentation build; supported Julia/platform combinations; aarch64 binary/FMA claims.
  • Prepared but unexecuted probes in scratch/probe.jl: independent dense-LU/residual checks across scaling modes, five element types and both index widths; row-scale values; preallocation/FMA/banded options; symbolic-only construction; transpose/adjoint solves; both refactor modes; duplicate validation and integer boundaries; bitwise default parity against origin/main; an analytic ForwardDiff derivative.
  • No repository Manifest or exact Julia-version lock was available. Runtime correctness and absence of regressions remain unestablished by this review. CI was not queried; the header preserves the supplied status.

AUTHOR_GRADE: {"task":"feature","score":4,"difficulty":1,"rounds":1,"author_harness":"unknown","author_model":"unknown","note":"Inspection establishes minimal documented keyword forwarding and sensible analytic pivot tests. Runic reproducibly rejects the new test layout. Host resource limits prevented numerical execution, so no runtime correctness claim is made."}

Head link: a53d13e


🤖 Posted by an AI agent — harness: codex-cli 0.164.0 · model: gpt-6.1-sol
Conversation: local session 01a1d25d-5355-7871-962c-f643f3edc088 (no session URL)

@ChrisRackauckas
ChrisRackauckas merged commit 7d267d6 into SciML:main Nov 15, 2026
20 checks passed
@ranocha

ranocha commented Nov 15, 2026 •

Copy link
Copy Markdown
Member Author

Thanks a lot for reviewing and merging this PR. I have already bumped the version in Project.toml and CI passes on main. Thus, it would be great if you could register the new version. Thanks again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants