Skip to content

Fix cholesky_inv documented argument name to match the binding - #3950

Open
ayaangazali wants to merge 1 commit into
ml-explore:mainfrom
ayaangazali:fix-cholesky-inv-argname
Open

Fix cholesky_inv documented argument name to match the binding#3950
ayaangazali wants to merge 1 commit into
ml-explore:mainfrom
ayaangazali:fix-cholesky-inv-argname

Conversation

@ayaangazali

Copy link
Copy Markdown
Contributor

Proposed changes

cholesky_inv advertises its first argument as L in both nb::sig and the docstring, but the bound keyword is a, so the documented call fails:

L = mx.linalg.cholesky(a, stream=mx.cpu)
mx.linalg.cholesky_inv(L=L, stream=mx.cpu)   # TypeError: incompatible function arguments
mx.linalg.cholesky_inv(a=L, stream=mx.cpu)   # works, but is not what the docs say

I fixed the signature and docstring to say a rather than renaming the bound argument, for two reasons: every other binding in linalg.cpp uses "a"_a, and renaming would break anyone already calling a=. Since the surrounding math uses L for the Cholesky factor and A for the original matrix, I also spelled out in the arg description that the input is the factor, so the rename does not read as "pass A here".

If you would rather go the other way and rename the bound argument to L, that is a one word change and I am happy to switch it.

Checklist

Put an x in the boxes that apply.

  • I have read the CONTRIBUTING document
  • I have run pre-commit run --all-files to format my code / installed pre-commit prior to committing changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the necessary documentation (if needed)

(docs only, no behavior change; rebuilt CPU-only and checked the rendered signature, test_linalg.py passes)


usual disclosure: i'm a freshman and Claude Code helps me catch this sort of drift, but i ran both calls above myself, checked the other linalg bindings to see which name was the odd one out, and rebuilt to confirm the signature renders correctly. glad to be told if you prefer the other direction.

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.

1 participant