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

Add certificate to is_relatively_complemented() #20972

Closed
jm58660 mannequin opened this issue Jul 7, 2016 · 16 comments
Closed

Add certificate to is_relatively_complemented() #20972

jm58660 mannequin opened this issue Jul 7, 2016 · 16 comments

Comments

@jm58660
Copy link
Mannequin

jm58660 mannequin commented Jul 7, 2016

At #20940 Kevin Dilks suggested a certificate-option, which I did. For consistency I guess that other functions should have the same option too. This patch adds it to is_relatively_complemented().

CC: @kevindilks

Component: combinatorics

Author: Jori Mäntysalo

Branch/Commit: 1213a37

Reviewer: Travis Scrimshaw

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

@jm58660 jm58660 mannequin added this to the sage-7.3 milestone Jul 7, 2016
@jm58660 jm58660 mannequin added c: combinatorics labels Jul 7, 2016
@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Jul 7, 2016

Branch: u/jmantysalo/rel_complemented_cert

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Jul 7, 2016

Commit: dcbdf00

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Jul 7, 2016

New commits:

dcbdf00Add certificate to is_relatively_complemented().

@jm58660 jm58660 mannequin added the s: needs review label Jul 7, 2016
@tscrim
Copy link
Collaborator

tscrim commented Jul 7, 2016

comment:3

I think the INPUT: block should just say:

- ``certificate`` -- (default: ``False``) whether to return a certificate if
  ``self`` is not relatively complemented

and then put the description of the certificate/output in an OUTPUT: block. Similar for #20940.

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Jul 8, 2016

comment:4

Replying to @tscrim:

I think the INPUT: block should just say:

- ``certificate`` -- (default: ``False``) whether to return a certificate if
  ``self`` is not relatively complemented

and then put the description of the certificate/output in an OUTPUT: block. Similar for #20940.

Can be done, of course, but I guess that then at least is_dismantlable() should be changed too. What about breadth() of a lattice? See also is_chordal() and is_circulant() in generic graphs.

Is it possible to accept this and #20940, and then later change docstrings for those? Also we should think if certify at canonical_label() of graphs should be changed to certificate.

@jm58660 jm58660 mannequin added s: needs info and removed s: needs review labels Jul 8, 2016
@tscrim
Copy link
Collaborator

tscrim commented Jul 8, 2016

comment:5

I think it is better to do this to new functionality in those tickets and then change the other docstrings in a separate ticket. This keeps changes local to the ticket, as well as we don't introduce something that is worse and we are just going to change one moment latter. This has a slight disadvantage of making the doc be less consistent, but Sage is not known for its doc consistency ;). (Yes, I know that is somewhat of a double standard, but the locality of change is the differentiating factor.)

Although my opinion on this is not very strong. If you think doc consistency is more important than doc correctness (or at least separation of concerns within the doc), then we can leave it alone.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 8, 2016

Changed commit from dcbdf00 to f8aca83

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 8, 2016

Branch pushed to git repo; I updated commit sha1. New commits:

f8aca83Restructure input-output blocks.

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Jul 8, 2016

comment:7

Do you mean something like this?

Replying to @tscrim:

I think it is better to do this to new functionality in those tickets and then change the other docstrings in a separate ticket. - - Sage is not known for its doc consistency ;). - -

Whatever. In any case we will need documentation polishing tickets sometimes, as functions will have diverse format for same things.

@jm58660 jm58660 mannequin added s: needs review and removed s: needs info labels Jul 8, 2016
@tscrim
Copy link
Collaborator

tscrim commented Jul 11, 2016

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Jul 11, 2016

comment:8

I would prefer if it is one bullet point, but that's only because I would use separate bullet points for different parts when the output is a tuple. Also, that is so stylistic and minor that I'll leave that decision to you.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 11, 2016

Branch pushed to git repo; I updated commit sha1. New commits:

1213a37Output block format.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 11, 2016

Changed commit from f8aca83 to 1213a37

@jm58660
Copy link
Mannequin Author

jm58660 mannequin commented Jul 11, 2016

comment:10

Replying to @tscrim:

I would prefer if it is one bullet point, but that's only because I would use separate bullet points for different parts when the output is a tuple.

I am not sure what you mean. Something like this?

@tscrim
Copy link
Collaborator

tscrim commented Jul 11, 2016

comment:11

Yep, exactly. Thanks.

@vbraun
Copy link
Member

vbraun commented Jul 12, 2016

Changed branch from u/jmantysalo/rel_complemented_cert to 1213a37

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

2 participants