-
Notifications
You must be signed in to change notification settings - Fork 2
Implement Kronecker product for AbstractDeviceSparseMatrix #25
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
Conversation
- Add kron kernel for COO format - Implement kron for AbstractDeviceSparseMatrix with automatic format conversion - Add comprehensive tests for kron with COO, CSC, CSR, and mixed formats - Fix bug in CSR sorting key calculation (was using m instead of n) Co-authored-by: albertomercurio <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25 +/- ##
==========================================
+ Coverage 84.34% 84.91% +0.57%
==========================================
Files 13 13
Lines 728 769 +41
==========================================
+ Hits 614 653 +39
- Misses 114 116 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@copilot the tests are failing. This is because you are using Also, I like that you defined the method for CSC, but I don't like that you defined a generic method for two AbstractDeviceSparseMatrix. You should only define the methods for CSC-CSC, CSR-CSR and COO-COO in their respective files. Also, you can write the kron coo kernel directly in the coo kernels file, rather than creating a new file. Finally, you don't need to define a new shared function for the tests specific for kron. You can add a new test set on each format. Also, don't use manually set matrices, but use rectangular random matrices of a sufficiently large size, like in the already existing tests. |
- Remove synchronize() call after kernel launch - Remove generic AbstractDeviceSparseMatrix kron method - Move kron kernel to matrix_coo_kernels.jl - Add separate kron methods for COO, CSC, and CSR in their respective files - Integrate kron tests into each format's test file - Use rectangular random matrices in tests - Remove standalone kron.jl and kron_kernels.jl files Co-authored-by: albertomercurio <[email protected]>
I've refactored the implementation:
Changes in commit e37ecbc Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Implementation Plan for Kronecker Product
kronfor each sparse matrix formatSummary
Refactored the kron implementation based on review feedback:
synchronize()call - The kernel doesn't need explicit synchronizationkron(AbstractDeviceSparseMatrix, AbstractDeviceSparseMatrix)kron(COO, COO),kron(CSC, CSC), andkron(CSR, CSR)in their respective filesmatrix_coo_kernels.jlinstead of standalone filekron.jlandkron_kernels.jlfilesThe implementation now follows the repository's structure and conventions.
Original prompt
AbstractDeviceSparseMatrix#19💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.