Opened 3 years ago

Closed 3 years ago

#20972 closed enhancement (fixed)

Add certificate to is_relatively_complemented()

Reported by: jmantysalo Owned by:
Priority: major Milestone: sage-7.3
Component: combinatorics Keywords:
Cc: kdilks Merged in:
Authors: Jori Mäntysalo Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 1213a37 (Commits) Commit: 1213a37113d442434e2dba2ef211f33fd9ec18e3
Dependencies: Stopgaps:

Description

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().

Change History (12)

comment:1 Changed 3 years ago by jmantysalo

  • Branch set to u/jmantysalo/rel_complemented_cert

comment:2 Changed 3 years ago by jmantysalo

  • Cc kdilks added
  • Commit set to dcbdf00ffcb2f05292ba3f3d336aa07843b13106
  • Status changed from new to needs_review

New commits:

dcbdf00Add certificate to is_relatively_complemented().

comment:3 follow-up: Changed 3 years ago by 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.

comment:4 in reply to: ↑ 3 Changed 3 years ago by jmantysalo

  • Status changed from needs_review to needs_info

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.

comment:5 follow-up: Changed 3 years ago by 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. 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.

comment:6 Changed 3 years ago by git

  • Commit changed from dcbdf00ffcb2f05292ba3f3d336aa07843b13106 to f8aca83288afbeea6a78ff1db2787b2a2f808234

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

f8aca83Restructure input-output blocks.

comment:7 in reply to: ↑ 5 Changed 3 years ago by jmantysalo

  • Status changed from needs_info to needs_review

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.

comment:8 follow-up: Changed 3 years ago by tscrim

  • Reviewers set to Travis Scrimshaw

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.

comment:9 Changed 3 years ago by git

  • Commit changed from f8aca83288afbeea6a78ff1db2787b2a2f808234 to 1213a37113d442434e2dba2ef211f33fd9ec18e3

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

1213a37Output block format.

comment:10 in reply to: ↑ 8 Changed 3 years ago by jmantysalo

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?

comment:11 Changed 3 years ago by tscrim

  • Status changed from needs_review to positive_review

Yep, exactly. Thanks.

comment:12 Changed 3 years ago by vbraun

  • Branch changed from u/jmantysalo/rel_complemented_cert to 1213a37113d442434e2dba2ef211f33fd9ec18e3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.