#27014 closed enhancement (fixed)

Deprecate sage.misc.misc.uniq

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.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) 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 17 months ago by jdemeyer

  • Dependencies set to #26933

comment:2 Changed 17 months ago by jdemeyer

  • Branch set to u/jdemeyer/deprecate_sage_misc_misc_uniq

comment:3 Changed 17 months ago by jdemeyer

  • Cc tscrim added
  • Commit set to 17ee0d00993df5b9b58ca814e31aa214cf6bb2d1
  • Status changed from new to needs_review

New commits:

0d45f46Do not sort in Subsets_s
dba4c44Make _stable_uniq private
17ee0d0Deprecate uniq()

comment:4 Changed 17 months ago by mantepse

  • 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 follow-up: Changed 17 months ago by tscrim

  • 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 (non-expert) user perspective. So I am somewhat inclined to also just do the _stable_uniq -> uniq now. Thoughts?

comment:6 in reply to: ↑ 5 ; follow-up: Changed 17 months ago by jdemeyer

  • 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 17 months ago by tscrim

  • 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 follow-up: Changed 17 months ago by mantepse

  • 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 ; follow-up: Changed 17 months ago by tscrim

Replying to mantepse:

Why not simply introduce the function stable_uniq, and remove uniq 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 time uniq?

Someone might be relying on this sorting behavior in the wild, so we need to deprecate it.

comment:10 in reply to: ↑ 9 Changed 17 months ago by mantepse

Replying to tscrim:

Replying to mantepse:

Why not simply introduce the function stable_uniq, and remove uniq 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 17 months ago by git

  • Commit changed from 17ee0d00993df5b9b58ca814e31aa214cf6bb2d1 to b4f67ca8620ffff6cf761abd44fe287a88c5bf13

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

b4f67caDeprecate uniq()

comment:12 Changed 17 months ago by jdemeyer

  • Status changed from needs_work to needs_review

Reworded deprecation message as Travis suggested.

comment:13 Changed 17 months ago by tscrim

  • Reviewers set to Martin Rubey, Travis Scrimshaw
  • Status changed from needs_review to positive_review

Thank you.

comment:14 Changed 17 months ago by embray

  • Milestone changed from sage-8.6 to sage-8.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 sage-pending or sage-wishlist.

comment:15 Changed 17 months ago by vbraun

  • Branch changed from u/jdemeyer/deprecate_sage_misc_misc_uniq to b4f67ca8620ffff6cf761abd44fe287a88c5bf13
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.