FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

ENH: `torch` dtype promotions by crusaderky · Pull Request #298 · data-apis/array-api-compat · GitHub

/ array-api-compat Public

ENH: torch dtype promotions - #298

Draft
crusaderky wants to merge 2 commits into
data-apis:mainfrom
crusaderky:torch_dtypes
ConversationCommits (2)ChecksFiles changed
Draft

ENH: torch dtype promotions#298
crusaderky wants to merge 2 commits into
data-apis:mainfrom
crusaderky:torch_dtypes

Conversation

crusaderky commented Apr 3, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor
  • On PyTorch >=2.3, support uint16, uint32, and uint64 in result_type
  • On PyTorch >=2.3, sum and prod will now promote uint8, uint16 and uint32 to uint64.

Copilot AI review requested due to automatic review settings April 3, 2025 10:35
Copilot AI reviewed Apr 3, 2025
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Pull Request Overview

This PR enhances dtype promotion behavior in PyTorch compatibility by supporting additional unsigned integer types and standardizing deep copy semantics for sum and prod operations when axis is an empty tuple.

  • Updated the promotion table to incorporate uint16, uint32, and uint64 on PyTorch ≥2.3.
  • Introduced a new internal helper (_sum_prod_no_axis) to implement deep-copy behavior for sum and prod when axis=().
Comments suppressed due to low confidence (2)

array_api_compat/torch/_aliases.py:280

  • Consider adding unit tests to explicitly verify the behavior of _sum_prod_no_axis across different dtypes, including the cases with and without _HAS_LARGE_UINT enabled.
def _sum_prod_no_axis(x: Array, dtype: DType | None) -> Array:

array_api_compat/torch/_aliases.py:280

  • [nitpick] Consider renaming _sum_prod_no_axis to a more descriptive name that conveys its role in returning a deep copy when no axis is provided, for example, _cast_and_clone_no_axis.
def _sum_prod_no_axis(x: Array, dtype: DType | None) -> Array:

ev-br commented Apr 3, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

#253 seems to have run into a wall with uint promotions in pytorch, did something change meanwhile in the meantime?

crusaderky commented Apr 3, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

#253 seems to have run into a wall with uint promotions in pytorch, did something change meanwhile in the meantime?

No, nothing has changed on the torch side AFAIK.

  • The changes to the dtypes table in this PR are functionally the same as in ENH: torch: unsigned types #253, but IMHO cleaner. They don't need to wait for full torch support in various functions to be merged.
  • The fix to sum(x, dtype=x.dtype, axis=() is unrelated to dtype promotion
  • The promotion of uint8/16/32 to uint64 in sum and prod is missing in ENH: torch: unsigned types #253, and something you want to have anyway
  • Unlike ENH: torch: unsigned types #253, this PR changes neither CI nor __array_namespace_info__, which is what makes that PR non-mergeable.

ev-br commented Apr 3, 2025

Copy link
Copy Markdown
Member

this PR changes neither CI nor array_namespace_info, which is what makes that PR non-mergeable.

You lost me here :-). So is it basically "let's add something because we want it, but not test it on CI because we know it'll break"?

crusaderky commented Apr 3, 2025

Copy link
Copy Markdown
Contributor Author

this PR changes neither CI nor array_namespace_info, which is what makes that PR non-mergeable.

You lost me here :-). So is it basically "let's add something because we want it, but not test it on CI because we know it'll break"?

Well, we will need this and the other PR eventually when torch introduces support.
We can either keep this PR unmerged, or leave it in main but untested by CI. Final users that somehow manage to avoid the many non-functioning APIs will benefit from it immediately.

crusaderky mentioned this pull request May 29, 2025
Open

ev-br commented Apr 7, 2025

Copy link
Copy Markdown
Member

We can either keep this PR unmerged, or leave it in main but untested by CI.

The latter we definitely do not want to do.

crusaderky marked this pull request as draft April 10, 2025 09:59
crusaderky mentioned this pull request Apr 10, 2025
Merged
crusaderky mentioned this pull request Apr 15, 2025
Closed
3 tasks

crusaderky commented Apr 15, 2025

Copy link
Copy Markdown
Contributor Author

I've updated this to incorporate the latest changes and added the contents of #253, which can now be closed.

crusaderky force-pushed the torch_dtypes branch 2 times, most recently from b5ca1be to 9cb105c Compare April 24, 2025 12:39
crusaderky mentioned this pull request May 15, 2025
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

Assignees

No one assigned

Labels

blocked by upstream

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL