Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Review: Batch of 10 functions from linalg ready for review #40

Closed
1 of 3 tasks
bmwoodruff opened this issue Jun 13, 2024 · 3 comments
Closed
1 of 3 tasks

Review: Batch of 10 functions from linalg ready for review #40

bmwoodruff opened this issue Jun 13, 2024 · 3 comments
Assignees
Labels

Comments

@bmwoodruff
Copy link
Collaborator

bmwoodruff commented Jun 13, 2024

Please review the examples in the following branch

Close when the following are complete:

@bmwoodruff bmwoodruff self-assigned this Jun 13, 2024
@bmwoodruff
Copy link
Collaborator Author

bmwoodruff commented Jun 13, 2024

(I'm going to have a conversation with myself to demonstrate what might happen when you ask for a review. Many times I'll just make change directly so we can pass this along to Numpy).

Remember to specify that you built numpy and the docs, and then ran the doctests after installing numpy.

spin build --clean
python -m pip install .
spin docs --clean
python tools/refguide_check.py --doctests

Everything built, so hooray! Remember to specific so I know it's working before I make changes.

Comments:

  • outer: Not sure if outer with negative is enough for another example. It does help someone see how the negatives propogate, so I'll leave it. Chuck may disagree (we can remove it then).
  • eig: The given matrix does NOT have repeated eigenvalues. I'll update the example with one that does and push it to your branch.
  • svd: Both examples of hermitian are NOT hermitian. I'll add a hermitian example.
  • svdvals: These work, but don't use new random number generation. I'll update.
  • matrix_rank: Remove type issues. Not sure if complex numbers is enough for another example, but it does add something slightly new.
  • det: Not sure the devs want the -2.0 changed. The zero matrix doesn't add much. I'm going to delete both.
  • trace: The length of line 3232 is way too long. I'll fix it.
  • matmul: All seem reasonable.
  • vecdot: looks good.

@bmwoodruff
Copy link
Collaborator Author

The changes all built. I've pushed them. Here's are two branches:

@charris, I'd love your feedback. Here's some questions I have:

  • What are your initial thoughts?
  • Is the commit trail for every function useful, or a waste of time?
    • Do you want a record of the AI contributed stuff to be separate from the human reviewed and adapted stuff, so we can tell what was AI, and what was human? Do you care?
    • Do you want a commit for each function that we add examples to, or would one large summary commit (rebase and squah) be better?
  • Would it be better to add fewer examples in each PR, or batch them together.
    • Here is a branch that @otieno-juma prepared which adds 3 examples to ma.convolve: numpy/numpy@main...otieno-juma:numpy:ai-examples-ma-convolve . It's ready for your review. I wanted you to have a single function, and batched version, of what we can provide, side-by-side.
    • Mattip mentioned last week to luxedo that he would rather review them in a batch. I see pros and cons to this. @InessaPawson was a bit worried about batching too much together. We will do whatever makes your life simpler for reviewing.
    • If we batch them together, do you have a preferred size?

We can run this AI tool across all of the Numpy code base. Some times AI will say a function has sufficient examples. Sometimes it returns utter garbage. Other times it finds nice extra examples. Often, it does a decent job of extending the existing examples.

  • I hope the hermitian flag for svd was useful, as well as repeated eigenvalues for eig. AI suggested the examples, which were trash, but a bit of revision from a human and done.
  • Some of the examples are lower quality, but I leave them in if I think they add something. For example using outer with a negative may help someone better understand how outer works. That example is definitely aimed at a newbie.
  • The interns have the freedom to toss low quality examples. I will toss ones that I see zero value in adding. I hope to get better at gauging "quality" as we get feedback.
  • I think the svdvals third example does a great job at illustrating how the svdvals are ordered from largest to smallest (with the diagonal matrix ordered small to large).

I don't want to overload the PR list (we're around 200, and I know you guys want to keep it to 150). That's why batching may be better.

  • How many per week do you think you have bandwidth for? I think we could get you examples for 30-40 functions per week right now. That could rise to much more as we work to fully automate branch creation. Today you have 8 +1 (I tossed 2 for very low quality).
  • I will be personally reviewing every branch. They should come to you squeaky clean. I'll squash, merge, rebase, etc. as directed, before we push something upstream as a PR.
  • I believe the original plan was to have you review things BEFORE we submit a branch as a PR. I'll keep to that model till directed otherwise. I hope tagging you @charris is sufficient, with links to the diff.

Thanks for being willing to be voluntold by @teoliphant to help us out here.

@bmwoodruff
Copy link
Collaborator Author

I'm closing this issue. It as designed as a practice run. I've gotten the needed feedback from the NumPy maintainers at the triage meetings.

@bmwoodruff bmwoodruff closed this as not planned Won't fix, can't repro, duplicate, stale Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant