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

oneunit of sparse matrix should return sparse matrix #30228

Merged
merged 7 commits into from
Dec 11, 2018
Merged

oneunit of sparse matrix should return sparse matrix #30228

merged 7 commits into from
Dec 11, 2018

Conversation

KlausC
Copy link
Contributor

@KlausC KlausC commented Dec 1, 2018

currently we have:

julia> oneunit(sparse([1 1 ; 2 2]))
2×2 Array{Int64,2}:
 1  0
 0  1
julia> one(sparse([1 1 ; 2 2]))
2×2 SparseMatrixCSC{Int64,Int64} with 2 stored entries:
  [1, 1]  =  1
  [2, 2]  =  1

where it should be. I think the behavour of one is as it should be, while oneunit is inconsistent.

julia> oneunit(sparse([1 2; 3 4]))
2×2 SparseMatrixCSC{Int64,Int64} with 2 stored entries:
  [1, 1]  =  1
  [2, 2]  =  1

julia> one(sparse([1 2; 3 4]))
2×2 SparseMatrixCSC{Int64,Int64} with 2 stored entries:
  [1, 1]  =  1
  [2, 2]  =  1

@JeffBezanson
Copy link
Member

Bumping for review. LGTM.

@KristofferC KristofferC added sparse Sparse arrays needs news A NEWS entry is required for this change minor change Marginal behavior change acceptable for a minor release labels Dec 5, 2018
@andreasnoack andreasnoack merged commit 5c5489e into JuliaLang:master Dec 11, 2018
KristofferC pushed a commit that referenced this pull request Dec 12, 2018
* added sprandn methods with Type

* oneunit of sparse matrix should return sparse array

(cherry picked from commit 5c5489e)
@KristofferC KristofferC mentioned this pull request Dec 12, 2018
52 tasks
@KlausC KlausC deleted the krc/oneunitsparse branch December 20, 2018 18:38
@ararslan
Copy link
Member

This has already been backported to 1.1.0 and cannot be backported to 1.0 as a minor change.

@ViralBShah
Copy link
Member

May I request not labelling this as a minor change and allowing it backported to 1.0? My only reason is that we are backporting a number of other sparse PRs that are small changes, but together greatly improve the overall quality of the sparse libraries.

There is no urgency, but it would be nice to get it into the 1.0.x at some point.

@KristofferC
Copy link
Member

We should backport bugfixes, doc improvements and performance improvements to patch releases. Since we are aiming to do 4 month releases it is not too bad if a small change like this goes into the next minor release imo.

@ViralBShah
Copy link
Member

ViralBShah commented Dec 31, 2018

I guess that is ok - even though this is probably a borderline bugfix. So folks who want LTS (the 1.0 branch) will not get this and have to wait for the next LTS.

@KristofferC
Copy link
Member

KristofferC commented Jan 1, 2019

So folks who want LTS (the 1.0 branch) will not get this and have to wait for the next LTS.

Yes, but the reason you are on an LTS in the first place likely because you don't want these type of changes until you decide to upgrade to a new minor version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor change Marginal behavior change acceptable for a minor release needs news A NEWS entry is required for this change sparse Sparse arrays
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants