Opened 2 years ago
Closed 2 years ago
#27014 closed enhancement (fixed)
Deprecate sage.misc.misc.uniq
Reported by:  jdemeyer  Owned by:  

Priority:  major  Milestone:  sage8.7 
Component:  misc  Keywords:  
Cc:  tscrim  Merged in:  
Authors:  Jeroen Demeyer  Reviewers:  Martin Rubey, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  b4f67ca (Commits, GitHub, GitLab)  Commit:  b4f67ca8620ffff6cf761abd44fe287a88c5bf13 
Dependencies:  #26933  Stopgaps: 
Description
uniq(X)
is just a shorthand for sorted(set(X))
: I don't think that we need a separate function for that. More seriously, the sorting is problematic since arbitrary sets may not be sortable in general, especially in Python 3 or after applying #22029.
Change History (15)
comment:1 Changed 2 years ago by
 Dependencies set to #26933
comment:2 Changed 2 years ago by
 Branch set to u/jdemeyer/deprecate_sage_misc_misc_uniq
comment:3 Changed 2 years ago by
 Cc tscrim added
 Commit set to 17ee0d00993df5b9b58ca814e31aa214cf6bb2d1
 Status changed from new to needs_review
comment:4 Changed 2 years ago by
 Reviewers set to Martin Rubey
 Status changed from needs_review to positive_review
I am all for it! (I ran the patchbot and looked at the code)
comment:5 followup: ↓ 6 Changed 2 years ago by
 Status changed from positive_review to needs_info
I am not so sure about actually deprecating it. The resulting behavior after _stable_uniq > uniq
is done will be the same is what it currently is. Plus the message seems like it will not be there after the deprecation, and I believe this is a useful function from the (nonexpert) user perspective. So I am somewhat inclined to also just do the _stable_uniq > uniq
now. Thoughts?
comment:6 in reply to: ↑ 5 ; followup: ↓ 7 Changed 2 years ago by
 Status changed from needs_info to positive_review
Replying to tscrim:
The resulting behavior after
_stable_uniq > uniq
is done will be the same is what it currently is.
No. The existing uniq()
sorts the list of objects, while _stable_uniq()
keeps the existing order.
comment:7 in reply to: ↑ 6 Changed 2 years ago by
 Status changed from positive_review to needs_work
Replying to jdemeyer:
Replying to tscrim:
The resulting behavior after
_stable_uniq > uniq
is done will be the same is what it currently is.No. The existing
uniq()
sorts the list of objects, while_stable_uniq()
keeps the existing order.
Ah, right, and the sorting of the output is documented. However, given the (eventual) goal, I think we should replace uniq
with _stable_uniq
and issue a deprecation warning about the sorting behavior change. Or at least in the current deprecation message say something like
the output of uniq(X) being sorted is deprecated; use sorted(set(X)) instead if you want the output sorted
The current deprecation message seems to indicate that the function will be removed altogether.
comment:8 followup: ↓ 9 Changed 2 years ago by
 Reviewers Martin Rubey deleted
Why not simply introduce the function stable_uniq
, and remove uniq
after deprecation?
Is there a good reason to call what's now _stable_uniq
in a year's time uniq
?
comment:9 in reply to: ↑ 8 ; followup: ↓ 10 Changed 2 years ago by
Replying to mantepse:
Why not simply introduce the function
stable_uniq
, and removeuniq
after deprecation?
stable_uniq
does not sound discoverable or natural IMO. Also for those people who only want the uniq
behavior, it means they would not have the change their code.
Is there a good reason to call what's now
_stable_uniq
in a year's timeuniq
?
Someone might be relying on this sorting behavior in the wild, so we need to deprecate it.
comment:10 in reply to: ↑ 9 Changed 2 years ago by
Replying to tscrim:
Replying to mantepse:
Why not simply introduce the function
stable_uniq
, and removeuniq
after deprecation?
stable_uniq
does not sound discoverable or natural IMO.
OK, although I wouldn't be looking for uniq
either. In fact, I was surprised by it's documentation, because the well known unix utility does something else.
I was looking for remove_duplicates
once, before it occurred to me that I want set
:)
Someone might be relying on this sorting behavior in the wild, so we need to deprecate it.
I think that we all agree on that.
comment:11 Changed 2 years ago by
 Commit changed from 17ee0d00993df5b9b58ca814e31aa214cf6bb2d1 to b4f67ca8620ffff6cf761abd44fe287a88c5bf13
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
b4f67ca  Deprecate uniq()

comment:12 Changed 2 years ago by
 Status changed from needs_work to needs_review
Reworded deprecation message as Travis suggested.
comment:13 Changed 2 years ago by
 Reviewers set to Martin Rubey, Travis Scrimshaw
 Status changed from needs_review to positive_review
Thank you.
comment:14 Changed 2 years ago by
 Milestone changed from sage8.6 to sage8.7
Retarging tickets optimistically to the next milestone. If you are responsible for this ticket (either its reporter or owner) and don't believe you are likely to complete this ticket before the next release (8.7) please retarget this ticket's milestone to sagepending or sagewishlist.
comment:15 Changed 2 years ago by
 Branch changed from u/jdemeyer/deprecate_sage_misc_misc_uniq to b4f67ca8620ffff6cf761abd44fe287a88c5bf13
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Do not sort in Subsets_s
Make _stable_uniq private
Deprecate uniq()