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 trim option to fkeep! and related functions #32972

Merged
merged 2 commits into from
Feb 21, 2020
Merged

Conversation

KristofferC
Copy link
Sponsor Member

@KristofferC KristofferC commented Aug 20, 2019

After #31724, the whole trim thing doesn't make sense since it is producing SparseArrays that are invalid according to our consistency checks.
Therefore, just remove the trim option.

AFAIU, we are also trying to move away from having an uninitialized "tail" in the different internal vectors as well.

Fixes #32967

@KristofferC KristofferC added kind:breaking This change will break code domain:arrays:sparse Sparse arrays status:triage This should be discussed on a triage call labels Aug 20, 2019
@KristofferC
Copy link
Sponsor Member Author

Triage because it is a bit unclear what to do here.

This is clearly breaking but the pr #31724 documents that the matrices generated by trim=false are invalid so I guess we should fix that... So should we put in a deprecation warning then..? But we don't want deprecations in 1.x etc.

@JeffBezanson JeffBezanson added the needs news A NEWS entry is required for this change label Aug 29, 2019
@KristofferC
Copy link
Sponsor Member Author

Triage: Check how much this is used in practice, and decide something based on that.

@KristofferC KristofferC self-assigned this Aug 29, 2019
@fredrikekre fredrikekre removed the status:triage This should be discussed on a triage call label Aug 29, 2019
@KristofferC
Copy link
Sponsor Member Author

@nanosoldier runtests(ALL; vs=":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

@KristofferC
Copy link
Sponsor Member Author

PkgEval looks clean, added a NEWS entry, will merge in a while unless I hear something.

@KristofferC KristofferC merged commit 5ecd1cf into master Feb 21, 2020
@KristofferC KristofferC deleted the kc/trim_sparse branch February 21, 2020 08:02
birm pushed a commit to birm/julia that referenced this pull request Feb 22, 2020
* remove trim option to fkeep! and related functions

* add NEWS entry
KristofferC added a commit that referenced this pull request Apr 11, 2020
* remove trim option to fkeep! and related functions

* add NEWS entry
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:arrays:sparse Sparse arrays kind:breaking This change will break code needs news A NEWS entry is required for this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

droptol! for SparseVector with trim=false does not work as expected
4 participants