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: AI Gen Examples for numpy.linalg.svdvals function #86

Closed
6 tasks
ogidig5 opened this issue Jun 17, 2024 · 7 comments
Closed
6 tasks

Review: AI Gen Examples for numpy.linalg.svdvals function #86

ogidig5 opened this issue Jun 17, 2024 · 7 comments
Assignees
Labels

Comments

@ogidig5
Copy link
Contributor

ogidig5 commented Jun 17, 2024

numpy/numpy@main...ogidig5:numpy:add-svdvals-example

Here's what reflects when the docs build:

Cleaned svdvals example

Checks

spin build && python -m pip install . && spin docs && python tools/refguide_check.py --doctests
  • All tests have passed
  • A visual inspection of the generated docs is correct

Acceptance Criteria:

Close when the following are complete, in order:

  • Reviewed by @bmwoodruff
  • Any suggestions to fork have been merged.
  • Reviewed by Numpy Mainter:

When all tasks are complete:

  • Submitted as PR to NumPy.
@ogidig5
Copy link
Contributor Author

ogidig5 commented Jun 17, 2024

Let me check the syntax of this function and make sure it runs well on the Jupyter Notebook. It should be a minor change.

@ogidig5 ogidig5 changed the title Review: AI Gen Examples for svdvals function Review: AI Gen Examples for linalg.svdvals function Jun 17, 2024
@ogidig5 ogidig5 changed the title Review: AI Gen Examples for linalg.svdvals function Review: AI Gen Examples for numpy.linalg.svdvals function Jun 17, 2024
@bmwoodruff
Copy link
Collaborator

Viewing the docs is not the same as running the doctests with python toos/refguide_check --doctests.

@ogidig5
Copy link
Contributor Author

ogidig5 commented Jun 18, 2024

I really need to quickly meet up with you. Seems I'm missing something crucial in the process. I want to work on that through the day

@ogidig5
Copy link
Contributor Author

ogidig5 commented Jun 19, 2024

I've been able to figure out the authentication issue. Here's the current changes pushed to my fork: numpy/numpy@main...ogidig5:numpy:add-svdvals-example

@bmwoodruff
Copy link
Collaborator

bmwoodruff commented Jun 19, 2024

You'll want to update the images to be current. Just edit your original message, so the new image shows up front.
Just about everything looks good now, with one issue:

  • I ran the doctests, and they failed.

Did you run the tests? In particular, did you run python tools/refguide_check.py --doctests. You must run all the tests. This should have failed on your machine as we didn't set a seed.

The tests failed because the matrix values are random. You have to add # may vary or # random after the output , otherwise the tests will fail. This is what the relevant code could look like.

...
    array([2.05175379, 0.75852089, 0.10363495]) # may vary
...
    array([[1.59357377, 0.2369699 , 0.03501063], # may vary
           [2.00939638, 0.39010965, 0.06555355]])
...
    array([2.35955923, 0.67914258, 0.15643921]) # may vary

Please run the doctests, and verify that the test fails before you make the change. If all you run is python tools/refguide_check.py --doctests, that isn't enough. You first have to build the docs, then install numpy, and then you can run that function. The entire string of commands you need is

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

AFTER you have verified the doctests failed, then make the change by adding # may vary. Then test them again (build numpy, install it, run the doc tests)

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

Then try changing # may vary to # random, and verify you pass the tests as well.

Glad you submitted this one. It's another thing I need to add to automation. I'll have to add a checker to see if "random" values are entered, and then add this to the first line of output.

@bmwoodruff
Copy link
Collaborator

Here is the error message I got:
Screenshot from 2024-06-19 13-49-38

@bmwoodruff
Copy link
Collaborator

I'm going to close this one. It isn't passing the tests, and the internship has ended. I think it's just missing the # may vary tag before it will pass the tests.

@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

2 participants