Skip to content

Fix copy bug in mlx.core.asarray - #4036

Closed
aaishwarymishra wants to merge 2 commits into
ml-explore:mainfrom
aaishwarymishra:asarray-bug
Closed

Fix copy bug in mlx.core.asarray#4036
aaishwarymishra wants to merge 2 commits into
ml-explore:mainfrom
aaishwarymishra:asarray-bug

Conversation

@aaishwarymishra

Copy link
Copy Markdown
Contributor

Please include a description of the problem or feature this PR is addressing. If there is a corresponding issue, include the issue #.

This pull request refines how the mx.asarray and mx.array functions handle the copy=False option when given an existing mx::array. Now, if the input array already matches the requested dtype, a copy is avoided as expected. If a dtype conversion would be needed, an error is raised if copy=False is specified. The test suite is updated to verify this improved behavior.

Improvements to array creation logic

  • Updated create_array in python/src/convert.cpp to only raise an error for copy=False when a dtype conversion is required, otherwise returning the original array without copying.
  • Removed redundant code handling for mx::array instances in the type dispatch, consolidating logic for clarity and correctness.

Test suite updates

  • Changed test_asarray_copy and test_asarray in python/tests/test_array.py to confirm that no error is raised when copy=False is used with an array of the same dtype, and that errors are still raised if a dtype conversion is required without copying. [1] [2]## Proposed changes

Checklist

Put an x in the boxes that apply.

  • I have read the CONTRIBUTING document
  • I have run pre-commit run --all-files to format my code / installed pre-commit prior to committing changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the necessary documentation (if needed)

@zcbenz zcbenz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The reason we throw for copy=False is that, semantically it means the copy would be shallow:

b = array(1)
a = asarray(b, copy=False)
b += 1
assert(a == 2)

which is not true in MLX because we don't have in-place updates, and accepting copy=False would just confuse users.

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