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

mutable poset: remove default for breaking ties in topological sort #26526

Closed
dkrenn opened this issue Oct 22, 2018 · 9 comments
Closed

mutable poset: remove default for breaking ties in topological sort #26526

dkrenn opened this issue Oct 22, 2018 · 9 comments

Comments

@dkrenn
Copy link
Contributor

dkrenn commented Oct 22, 2018

At the moment shells_topological of a mutable poset breaks ties by means of repr by default. However, mutable poset is a data structure and should not do this expensive operation by default, therefore this default should be removed which is the aim of this ticket.

Note that this default was in mainly due to convenience reasons to have the results in the doctests always in the same order.

(Motivation: For my code/application, this improves the computation time by more than a factor 2.)

CC: @behackl

Component: asymptotic expansions

Author: Daniel Krenn

Branch/Commit: fd98fc1

Reviewer: Benjamin Hackl

Issue created by migration from https://trac.sagemath.org/ticket/26526

@dkrenn
Copy link
Contributor Author

dkrenn commented Oct 22, 2018

Branch: u/dkrenn/repr-default-topological

@dkrenn
Copy link
Contributor Author

dkrenn commented Oct 22, 2018

Commit: fd98fc1

@dkrenn
Copy link
Contributor Author

dkrenn commented Oct 22, 2018

New commits:

fd98fc1Trac #26526: remove default "repr" for topological sorting

@dkrenn
Copy link
Contributor Author

dkrenn commented Oct 22, 2018

Author: Daniel Krenn

@behackl
Copy link
Member

behackl commented Apr 17, 2019

Reviewer: Benjamin Hackl

@behackl
Copy link
Member

behackl commented Apr 17, 2019

comment:3

I looked through the changes and they look good to me overall.

Would it make sense to add another doctest (possibly with output marked as random) where no key is passed and thus no sorting happens?

@dkrenn
Copy link
Contributor Author

dkrenn commented May 14, 2019

comment:4

Replying to @behackl:

Would it make sense to add another doctest (possibly with output marked as random) where no key is passed and thus no sorting happens?

I do not see much of a point adding such a test.

@behackl
Copy link
Member

behackl commented May 16, 2019

comment:5

Replying to @dkrenn:

Replying to @behackl:

Would it make sense to add another doctest (possibly with output marked as random) where no key is passed and thus no sorting happens?

I do not see much of a point adding such a test.

I was thinking about testing whether calling the method without a key still works -- but you are right, I also cannot think of a usecase for that.

Good to go.

@vbraun
Copy link
Member

vbraun commented May 21, 2019

Changed branch from u/dkrenn/repr-default-topological to fd98fc1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants