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.

Sign up for GitHub

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

Jump to bottom

Deprecate sage.misc.misc.uniq #27014

Closed
jdemeyer opened this issue Jan 4, 2019 · 20 comments
Closed

Deprecate sage.misc.misc.uniq #27014

jdemeyer opened this issue Jan 4, 2019 · 20 comments
Labels
p: major / 3 t: enhancement
Milestone
sage-8.7

Comments

@jdemeyer
Copy link

jdemeyer commented Jan 4, 2019

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.

Depends on #26933

CC: @tscrim

Component: misc

Author: Jeroen Demeyer

Branch/Commit: b4f67ca

Reviewer: Martin Rubey, Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/27014
@jdemeyer jdemeyer added this to the sage-8.6 milestone Jan 4, 2019
@jdemeyer
Copy link
Author

jdemeyer commented Jan 4, 2019

Dependencies: #26933

@jdemeyer
Copy link
Author

jdemeyer commented Jan 4, 2019

Branch: u/jdemeyer/deprecate_sage_misc_misc_uniq

@jdemeyer
Copy link
Author

jdemeyer commented Jan 4, 2019

Commit: 17ee0d0

@jdemeyer
Copy link
Author

jdemeyer commented Jan 4, 2019

New commits:

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

@mantepse
Copy link
Collaborator

mantepse commented Jan 4, 2019

Reviewer: Martin Rubey

@mantepse
Copy link
Collaborator

mantepse commented Jan 4, 2019

comment:4

I am all for it! (I ran the patchbot and looked at the code)

@tscrim
Copy link
Collaborator

tscrim commented Jan 4, 2019

comment:5

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?

@jdemeyer
Copy link
Author

jdemeyer commented Jan 4, 2019

comment:6

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.

@tscrim
Copy link
Collaborator

tscrim commented Jan 4, 2019

comment:7

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.

@mantepse
Copy link
Collaborator

mantepse commented Jan 4, 2019

Changed reviewer from Martin Rubey to none

@mantepse
Copy link
Collaborator

mantepse commented Jan 4, 2019

comment:8

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?

@tscrim
Copy link
Collaborator

tscrim commented Jan 5, 2019

comment:9

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.

@mantepse
Copy link
Collaborator

mantepse commented Jan 5, 2019

comment:10

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 5, 2019

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

b4f67caDeprecate uniq()

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 5, 2019

Changed commit from 17ee0d0 to b4f67ca

@jdemeyer
Copy link
Author

jdemeyer commented Jan 5, 2019

comment:12

Reworded deprecation message as Travis suggested.

@tscrim
Copy link
Collaborator

tscrim commented Jan 5, 2019

comment:13

Thank you.

@tscrim
Copy link
Collaborator

tscrim commented Jan 5, 2019

Reviewer: Martin Rubey, Travis Scrimshaw

@embray
Copy link
Contributor

embray commented Jan 15, 2019

comment:14

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.

@embray embray modified the milestones: sage-8.6, sage-8.7 Jan 15, 2019
@vbraun
Copy link
Member

vbraun commented Jan 22, 2019

Changed branch from u/jdemeyer/deprecate_sage_misc_misc_uniq to b4f67ca

@vbraun vbraun closed this as completed in 4fb44bc Jan 22, 2019
@jdemeyer jdemeyer mentioned this issue Jan 28, 2019
Closed
vbraun pushed a commit that referenced this issue Jun 21, 2023
gh-35564: sage.misc.misc, sage.combinat: Modularization fixes
3aa3644 
    
<!-- Please provide a concise, informative and self-explanatory title.
-->
<!-- Don't put issue numbers in the title. Put it in the Description
below. -->
<!-- For example, instead of "Fixes #12345", use "Add a new method to
multiply two integers" -->

### 📚 Description

<!-- Describe your changes here in detail. -->
<!-- Why is this change required? What problem does it solve? -->
- move `powerset` (= `subsets`) from the problematic `sage.misc.misc`
module to `sage.combinat.subset`,
- likewise we move the implementation of `uniq`
(`sage.misc.misc._stable_uniq`) and finish the job from #27014,
- we remove `sage.misc.misc.union`, deprecated in #16604,
- some other fixes to imports.

<!-- If this PR resolves an open issue, please link to it here. For
example "Fixes #12345". -->
Part of:
- #29705
<!-- If your change requires a documentation PR, please link it
appropriately. -->

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. It should be `[x]` not `[x
]`. -->

- [x] The title is concise, informative, and self-explanatory.
- [x] The description explains in detail what this PR is about.
- [x] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on
- #12345: short description why this is a dependency
- #34567: ...
-->
- Depends on #35672

<!-- If you're unsure about any of these, don't hesitate to ask. We're
here to help! -->
    
URL: #35564
Reported by: Matthias Köppe
Reviewer(s): David Coudert, Matthias Köppe
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
p: major / 3 t: enhancement
Projects
None yet
Milestone
sage-8.7
Development

No branches or pull requests
5 participants
@embray @vbraun @tscrim @jdemeyer @mantepse