possee-org / genai-numpy

MIT License
4 stars 6 forks source link

Review: AI Gen Examples for ma.dot, ma.cumsum and ma.prod #87

Open jud-sdev opened 1 week ago

jud-sdev commented 1 week ago

https://github.com/numpy/numpy/compare/main...jud-sdev:numpy:ma-func Here are the screensots of the compiled docs madot

macumsum

maprod

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:

bmwoodruff commented 1 week ago

ma.dot is on line 7984 of the original file. You've added examples to the wrong place. Check the file again, and you'll notice it appears in two places.

ma.cumsum

ma. prod

bmwoodruff commented 1 week ago

Did you run the doctests?

bmwoodruff commented 1 week ago

Your work on ma.cumsum has helped me find more functions that we can search through. Typing help(np.ma.cumsum) shows Help on _frommethod in module numpy.ma.core. This is the same message which appears with ma.prod.

Just wondering, why did you pick these two functions in particular? Did you look for ones that the script missed, or was it arbitrary?

jud-sdev commented 1 week ago

I ran doctests. In our conversation last week, you said ma has so many functions, and I can pick any function of my choice to practice with. So, I picked those three functions. Two of them are without examples while the third function has one example. For the ma.dot, I must have missed the lines, I'll put the examples where they're supposed to be. There were words between the examples, but I removed them. I'll include the words in the next update.

bmwoodruff commented 1 week ago

Thanks for the reply I was wondering if you found some way to locate these specific functions (they have an odd type that I didn't know to search for). The ma module is huge, so it's fine you chose these ones. I'm glad you did, because it showed me we were missing some things.

The doctest question I asked because you didn't check that box.

I'd love to have logs for these in on this site. Please add them and submit a PR to genai-numpy.

When you see a function with 7 examples provided, you'll have to start deciding if they are all really needed. If you see some that are super similar, then trim it down. This is subjective, so I think over time we'll learn how to trim things down.

jud-sdev commented 1 week ago

I've made the suggested changes. Meanwhile, I got indentation warnings/errors when I built the doc. I tried adjusting it, but it's not still working. I'll need your help on this. I have the three functions in 1 log file. Since I've already created the log on Thurday, before the latest instruction of having 1 log per function. Here the link to the log: https://github.com/possee-org/genai-numpy/blob/e949a3f321a881288513d989d8d2885a4ac3096d/examples/log/np_ma/ma.ex.log

bmwoodruff commented 1 week ago

When you get errors and aren't sure what they mean, please put them in a code block and share them.

Paste errors here

That can help identify the issue. I'll checkout your branch now and run the tests on my machine to see if I can find the issue.

bmwoodruff commented 1 week ago

@jud-sdev , look at line 5179 in https://github.com/numpy/numpy/compare/main...jud-sdev:numpy:ma-func. You can see quickly that you added spaces to a blank line, that should not have spaces added.

You will have other errors when you build things. Namely, the words between examples must be separated by a blank line from the examples. So instead of

        Multiple axes:
        >>> np.ma.prod(np.ma.array([[[1, 2], [3, 4]], [[5, 6], [7, 8]]]), axis=(0, 2))
        masked_array(data=[ 60, 672],
            mask=False,
            fill_value=999999)

you need

        Multiple axes:

        >>> np.ma.prod(np.ma.array([[[1, 2], [3, 4]], [[5, 6], [7, 8]]]), axis=(0, 2))
        masked_array(data=[ 60, 672],
            mask=False,
            fill_value=999999)

AI did not get proper spacing. This is something we have to clean up. If you run this, and then try to view the docs, you'll find it looks horrible.

bmwoodruff commented 1 week ago

One last comment: Use this link

The only difference is the ?expand=1 at the end of the URL.

jud-sdev commented 1 week ago

Thank you @bmwoodruff . I'll make the corrections and spin the docs and run the tests again.

jud-sdev commented 1 week ago

It worked fine now and passed the tests. Thank you. https://github.com/numpy/numpy/compare/main...jud-sdev:numpy:ma-func

bmwoodruff commented 1 week ago

This looked great to me, and then I ran one more test which I didn't list above (glad you helped me realize we must include it).

spin lint

Try running that test and you'll see that there are 4 lines that are too long. Those 4 lines need to be shortened. One of the lines was original code that you modified (the first). Update the file to make sure the lines are the proper length, and run the tests again.

Once you have finished that, it's time to learn how to rebase and squash to main, squashing all your commits to one. That way there is only one commit showing when you submit a PR.

Here's an example of how you can create a new branch on which to practice rebasing.

git checkout main
git pull upstream main --tags
git submodule update --init
git checkout ma-func
git checkout -b ma-func-squashed

The above makes sure you have the most recent version of main from numpy, and then creates a new branch ma-fun-squashed based on ma-func (that's why we did checkout ma-func first). Because we are on a new branch, if you mess anything up along the way, it's easy to just do a git branch -D ma-func-squashed to remove the branch, and then start again.

Type the following:

git log

You'll see that you have many commits. It would be nice if this were a single commit.

Now we rebase to main and squash the commits into a single commit.

git rebase -i main

This will open an editor showing all the commits you have back to main. Leave the first row alone. In the other rows, change pick to squash. Then save the file (Ctrl + S) and exit the editor (Ctrl + X). A second window will appear that contains all the commit messages along the way. You can now write a single simple commit message, deleting everything else. When you save and exit, git will squash everything to a single commit. View that your work is now just one commit away from main.

git log

Now delete the branch ma-func-squashed and try this again. Make up another name for a branch, base if on your ma-func branch, and practice squashing to main. Delete this new practice branch when you're done, and repeat it a few times.

When you feel comfortable with all this, checkout ma-func and squash to main git rebase -i main. If everything goes well, then can push the changes online with

git push -f origin ma-func

The -f is required, as you have to force push the changes over the top of what's currently online.

jud-sdev commented 1 week ago

Thank you. I'll work on that.

jud-sdev commented 1 week ago

I've corrected the line length issue and I've squashed all the commits into one. Thanks Ben for all the helps. I'm really learning alot. https://github.com/numpy/numpy/compare/main...jud-sdev:numpy:ma-func