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.

Sign up for GitHub

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

Jump to bottom

Format specification API #792

Merged
merged 16 commits into from
Oct 22, 2024
Merged

Format specification API #792

merged 16 commits into from
Oct 22, 2024

Conversation

hameerabbasi
Copy link
Collaborator

No description provided.

@hameerabbasi hameerabbasi marked this pull request as draft October 14, 2024 16:04
@hameerabbasi hameerabbasi self-assigned this Oct 14, 2024
@hameerabbasi hameerabbasi added enhancement Indicates new feature requests maintenance labels Oct 14, 2024
@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Oct 14, 2024 •
edited
Loading

CodSpeed Performance Report

Merging #792 will not alter performance

Comparing hameerabbasi:format-refactor (f7162f8) with main (ef53f7d)

Summary

✅ 340 untouched benchmarks

@hameerabbasi hameerabbasi force-pushed the format-refactor branch 2 times, most recently from 961d7a4 to 6ab8320 Compare October 17, 2024 13:36
@hameerabbasi hameerabbasi marked this pull request as ready for review October 22, 2024 08:46
@mtsokol
Copy link
Collaborator

mtsokol commented Oct 22, 2024

I see that the PR removes reshape and broadcast_to. Is it intended? I think these functions should stay as they are a part of Array API standard.

@hameerabbasi
Copy link
Collaborator Author

hameerabbasi commented Oct 22, 2024 •
edited
Loading

I see that the PR removes reshape and broadcast_to. Is it intended? I think these functions should stay as they are a part of Array API standard.

I was planning on adding them back as part of a follow-up PR, with some heuristics on the format selection.

@hameerabbasi hameerabbasi mentioned this pull request Oct 22, 2024
Open
@hameerabbasi hameerabbasi changed the title Format Generation Format specification API Oct 22, 2024
@hameerabbasi hameerabbasi enabled auto-merge (squash) October 22, 2024 09:56
mtsokol
mtsokol previously approved these changes Oct 22, 2024
View reviewed changes
Copy link
Collaborator

@mtsokol mtsokol left a comment

Choose a reason for hiding this comment

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

I really like this change! LGTM!

Only two nitpick, and one comment.

sparse/mlir_backend/_array.py Outdated Show resolved Hide resolved
sparse/mlir_backend/_array.py Outdated Show resolved Hide resolved
sparse/mlir_backend/levels.py Show resolved Hide resolved
@mtsokol
Copy link
Collaborator

mtsokol commented Oct 22, 2024

I see that the PR removes reshape and broadcast_to. Is it intended? I think these functions should stay as they are a part of Array API standard.

I was planning on adding them back as part of a follow-up PR, with some heuristics on the format selection.

Ok, I agree that they can be added first after merging it.

@hameerabbasi hameerabbasi mentioned this pull request Oct 22, 2024
Open
@hameerabbasi hameerabbasi merged commit 0a0802e into pydata:main Oct 22, 2024
16 of 18 checks passed
@hameerabbasi hameerabbasi deleted the format-refactor branch October 22, 2024 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtsokol mtsokol mtsokol approved these changes

Assignees

@hameerabbasi hameerabbasi

Labels
enhancement Indicates new feature requests maintenance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hameerabbasi @mtsokol