possee-org / genai-numpy

MIT License
4 stars 6 forks source link

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

Open ogidig5 opened 1 week ago

ogidig5 commented 1 week ago

https://github.com/numpy/numpy/compare/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

Acceptance Criteria:

Close when the following are complete, in order:

When all tasks are complete:

ogidig5 commented 1 week ago

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

bmwoodruff commented 1 week ago

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

ogidig5 commented 1 week ago

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 commented 1 week ago

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

bmwoodruff commented 1 week ago

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:

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 commented 1 week ago

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