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.

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

remove eltype interface-piracy #385

Merged
merged 1 commit into from
May 3, 2023
Merged

remove eltype interface-piracy #385

merged 1 commit into from
May 3, 2023

Conversation

thchr
Copy link
Contributor

@thchr thchr commented May 2, 2023

These specializations are unnecessary since the eltype(Type{AbstractArray{T}}) where T fallback already ensures this behavior; i.e., the interface of the supertype already provides this.

Removing this was motivated by a discussion in the ttfx Slack channel where vtjnash indicated that these types of superfluous interface-extensions could be considered a form of piracy.

This addresses the following entry from a @snoopr report:

inserting eltype(::Type{SparseArrays.CHOLMOD.Dense{T}}) where T<:Union{Float64, ComplexF64} @ SparseArrays.CHOLMOD ~/dev/julia-master/share/julia/stdlib/v1.10/SparseArrays/src/solvers/cholmod.jl:983 invalidated:
   backedges: 1: superseding eltype(::Type{<:AbstractArray{E}}) where E @ Base abstractarray.jl:239 with MethodInstance for eltype(::Type{<:AbstractArray{T, N}} where {T, N}) (4 children)

@codecov-commenter
Copy link

Codecov Report

Merging #385 (07532b2) into main (5fc5771) will decrease coverage by 0.01%.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #385      +/-   ##
==========================================
- Coverage   93.86%   93.86%   -0.01%     
==========================================
  Files          12       12              
  Lines        7487     7484       -3     
==========================================
- Hits         7028     7025       -3     
  Misses        459      459              
Impacted Files Coverage Δ
src/solvers/cholmod.jl 90.65% <ø> (-0.04%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@thchr
Copy link
Contributor Author

thchr commented May 2, 2023

Test failures are the same as seen in #380, i.e., unrelated.

@SobhanMP SobhanMP requested a review from rayegun May 3, 2023 04:23
Copy link
Member

@rayegun rayegun left a comment

Choose a reason for hiding this comment

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

Fine, I'm not aware of any reason to keep these if they're all subtypes of AbstractArray

@rayegun rayegun merged commit 0773db8 into JuliaSparse:main May 3, 2023
@thchr thchr deleted the patch-1 branch May 3, 2023 05:06
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.

3 participants