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

Make Autograph support in-place assignments with static slice(start, stop, step) #843

Merged
merged 9 commits into from
Jul 15, 2024

Conversation

tzunghanjuang
Copy link
Collaborator

@tzunghanjuang tzunghanjuang commented Jun 20, 2024

Context:
Make set_item primitive detect slice. If yes, use dynamic_update_slice.
Must be used with PennyLaneAI/diastatic-malt#6.

Update (2024/07/12): We no longer consider dynamic_update_slice as an option since it produces unwanted results if the indices are out-of-range.

Description of the Change:

  • Malt package has a new implementation for slice(start, stop, step). We have to capture such arguments.

Possible Drawbacks:
If the length of assignment does not match, jax.lax.dynamic_update_slice will adjust the starting point.

  • Only static slices are accepted.
result = jnp.empty((6, ), dtype=x.dtype)
result[::2] = jnp.ones((3,))
# result will be [1, 0, 1, 0, 1,0 ]

Related GitHub Issues:
#516
[sc-60315]

tzunghanjuang referenced this pull request in PennyLaneAI/diastatic-malt Jul 10, 2024
**Context:**
Make ``y[start: stop] = x`` work.
Must be used with ``https://github.com/PennyLaneAI/catalyst/pull/843``.

**Description of the Change:**

**Benefits:**

**Possible Drawbacks:**

**Related GitHub Issues:**

PennyLaneAI/catalyst#516 (comment)
[sc-60315]
@dime10
Copy link
Contributor

dime10 commented Jul 10, 2024

We might want to push a new release of malt before merging this PR :) @josh146

@dime10
Copy link
Contributor

dime10 commented Jul 12, 2024

@tzunghanjuang This needs an update because we decided against using dynamic_update_slice right?

@tzunghanjuang
Copy link
Collaborator Author

tzunghanjuang commented Jul 12, 2024

@dime10 Yes, I am still looking for the alternative.

@dime10
Copy link
Contributor

dime10 commented Jul 12, 2024

@dime10 Yes, I am still looking for the alternative.

Only allowing static indices right?

@tzunghanjuang
Copy link
Collaborator Author

Only allowing static indices right?

Yes

@tzunghanjuang tzunghanjuang changed the title Fix IndexError related to dynamic slice with jax and autograph Fix IndexError related to slice (Static Only) with jax and autograph Jul 12, 2024
@tzunghanjuang
Copy link
Collaborator Author

@dime10 I have updated the set_item primitive and tests.

@tzunghanjuang tzunghanjuang changed the title Fix IndexError related to slice (Static Only) with jax and autograph Make autograog support slice(start, stop, step) Jul 12, 2024
@tzunghanjuang tzunghanjuang changed the title Make autograog support slice(start, stop, step) Make autograoh support slice(start, stop, step) Jul 12, 2024
@tzunghanjuang tzunghanjuang changed the title Make autograoh support slice(start, stop, step) Make autograph support slice(start, stop, step) Jul 12, 2024
@tzunghanjuang tzunghanjuang changed the title Make autograph support slice(start, stop, step) Make Autograph support slice(start, stop, step) Jul 12, 2024
Copy link
Contributor

@dime10 dime10 left a comment

Choose a reason for hiding this comment

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

Nice! Just needs a changelog and a minimum version bump on the malt dependency :)

@dime10
Copy link
Contributor

dime10 commented Jul 15, 2024

The malt package has been updated btw :) https://pypi.org/project/diastatic-malt/

@tzunghanjuang
Copy link
Collaborator Author

HI @dime10. What should be the category for PR? (Feature, improvement or bug fix?)

@dime10
Copy link
Contributor

dime10 commented Jul 15, 2024

HI @dime10. What should be the category for PR? (Feature, improvement or bug fix?)

Usually we don't triage PRs by what kind they are (although if you really want to you can). Instead, we assign which part of the project the PR is modifying (docs, ci, frontend, compiler, runtime).

The classification (improvement, bug, etc) is usually used for issues.

EDIT: Oh are you referring to the changelog? In that case I think it is an improvement (since it improves the existing feature array assignment feature) :)

Copy link
Contributor

@dime10 dime10 left a comment

Choose a reason for hiding this comment

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

Thanks @tzunghanjuang :)

doc/changelog.md Outdated Show resolved Hide resolved
@tzunghanjuang tzunghanjuang changed the title Make Autograph support slice(start, stop, step) Make Autograph support in-place assignments wiht slice(start, stop, step) Jul 15, 2024
@tzunghanjuang tzunghanjuang changed the title Make Autograph support in-place assignments wiht slice(start, stop, step) Make Autograph support in-place assignments with static slice(start, stop, step) Jul 15, 2024
Copy link

codecov bot commented Jul 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.99%. Comparing base (93c05e5) to head (8e48834).
Report is 227 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #843   +/-   ##
=======================================
  Coverage   97.98%   97.99%           
=======================================
  Files          71       71           
  Lines       10546    10548    +2     
  Branches      960      961    +1     
=======================================
+ Hits        10334    10336    +2     
  Misses        169      169           
  Partials       43       43           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tzunghanjuang tzunghanjuang merged commit ceafc68 into main Jul 15, 2024
43 checks passed
@tzunghanjuang tzunghanjuang deleted the autograph-slice branch July 15, 2024 20:36
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.

2 participants