Skip to content

updated arange to mgrid in tutorials and references #71

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mm-aws06
Copy link

@mm-aws06 mm-aws06 commented Apr 22, 2025

Issue #, if available:

<Fill ME>

Description of changes:

Updated arange to mgrid in tutorials and references

Testing:

Please see detailed unit test requirements in the CONTRIBUTING.md

  • The change is covered by numeric check using nki.baremetal
  • The change is covered by performance benchmark test using nki.benchmark
  • The change is covered by end-to-end integration test

Pull Request Checklist

  • I have filled in all the required field in the template
  • I have tested locally that all the tests pass
  • By submitting this pull request, I confirm that my contribution is made under the terms of the MIT-0 license.

aws-qieqingy
aws-qieqingy previously approved these changes Apr 23, 2025
JonathanHenson
JonathanHenson previously approved these changes Apr 23, 2025

if_reduction = nl.arange(reduction_size)[None, :]
_, if_reduction = nl.mgrid[0:1, 0:reduction_size]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check the AutoTest error message, this line seems to have indentation error.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed indentation and pushed again :)

@mm-aws06 mm-aws06 dismissed stale reviews from JonathanHenson and aws-qieqingy via 7296142 April 23, 2025 17:23
@mm-aws06
Copy link
Author

mm-aws06 commented May 1, 2025

After reviewing my PR results, there's something I'd like to ask for anyone's opinion on. The changes I made from arange to mgrid in allocated_attention.py are correct (from what I can tell), however the indexing process is different:

arange and mgrid are both used to generate ranges of indices, but they return different shapes and types:
arange(N) returns a 1D array: [0, 1, ..., N-1]
mgrid[0:N, 0:M] returns two 2D arrays: one for row indices, one for column indices, both of shape (N, M)

Should I switch back from mgrid to arange here? Or alter the indexing? (and for the latter, will that involve making changes to the test files as well?)

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.

3 participants