-
Notifications
You must be signed in to change notification settings - Fork 2
Force fields to have same eltype #28
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
base: main
Are you sure you want to change the base?
Conversation
|
🤖 Hi @albertomercurio, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #28 +/- ##
==========================================
- Coverage 82.64% 82.40% -0.25%
==========================================
Files 14 13 -1
Lines 801 773 -28
==========================================
- Hits 662 637 -25
+ Misses 139 136 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📋 Review Summary
This pull request improves type stability by enforcing element type consistency in the sparse array constructors. The removal of Reactant.jl support is also noted. The changes are generally good, but there are a few minor suggestions for improving consistency and readability.
🔍 General Feedback
- The use of
copyin the constructors is inconsistent across the different sparse matrix types. It's recommended to make this behavior consistent. - The docstrings for the sparse matrix types could be simplified for better readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Benchmark Results'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.30.
| Benchmark suite | Current: a8875c4 | Previous: c26f8a5 | Ratio |
|---|---|---|---|
Sparse Vector/JLArray/Sum |
22782 ns |
16145.5 ns |
1.41 |
Kronecker Product/Array/CSC |
539683 ns |
407425 ns |
1.32 |
This comment was automatically generated by workflow using github-action-benchmark.
gdalle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be enough to get Reactant working?
|
Unfortunately the type relaxation didn't help. |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Fixes #10.
There are still some problems with Reactant.jl though.