Opened 5 years ago

Closed 5 years ago

#20940 closed enhancement (fixed)

LatticePoset: add is_sectionally_complemented()

Reported by: jmantysalo Owned by:
Priority: major Milestone: sage-7.4
Component: combinatorics Keywords:
Cc: tscrim Merged in:
Authors: Jori Mäntysalo Reviewers: Kevin Dilks
Report Upstream: N/A Work issues:
Branch: b198cee (Commits, GitHub, GitLab) Commit: b198ceee80bc66133224027a0ca75a1251d684ea
Dependencies: Stopgaps:

Status badges

Description

This patch will add a function that checks if the lattice is sectionally complemented or not.

Change History (28)

comment:1 Changed 5 years ago by jmantysalo

  • Branch set to u/jmantysalo/sectionally_complemented

comment:2 Changed 5 years ago by jmantysalo

  • Commit set to 18e864d926d07cec25c733701452091b5b971e49
  • Status changed from new to needs_review

New commits:

18e864dAdd is_sectionally_complemented().

comment:3 follow-up: Changed 5 years ago by kdilks

Would it be of any value to have an optional certificate parameter which would return the poset element whose principal order ideal isn't complemented instead of just False?

comment:4 Changed 5 years ago by git

  • Commit changed from 18e864d926d07cec25c733701452091b5b971e49 to 23c3ee3f5b23192f2d60f2979865d4baf14f60ca

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

23c3ee3Add certificate-option.

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

Replying to kdilks:

Would it be of any value to have an optional certificate parameter which would return the poset element whose principal order ideal isn't complemented instead of just False?

As you wish.

But now for orthogonality we should add certificates to other functions too. Which is not a bad idea at all, at least for demonstration purposes (really show the pentagon for non-modular lattice etc.). I could start with complements(), but then someone should first review #20727 that corrects a trivial bug.

comment:6 Changed 5 years ago by jmantysalo

See also #20972. Hope that certificate-options do something similar and expectable.

comment:7 Changed 5 years ago by jmantysalo

  • Status changed from needs_review to needs_work

See comment about certificate and INPUT-OUTPUT -sections at #20972.

Last edited 5 years ago by jmantysalo (previous) (diff)

comment:8 Changed 5 years ago by git

  • Commit changed from 23c3ee3f5b23192f2d60f2979865d4baf14f60ca to 8f1c6694856986261bf6b7d90622d65079b12e17

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

8f1c669Reformatted input-output -blocks.

comment:9 follow-up: Changed 5 years ago by jmantysalo

  • Cc tscrim added
  • Status changed from needs_work to needs_info

Travis, I reformatted the input-output -blocks like in #20972 (btw, thanks for review).

But should the certificate be one object, i.e. a tuple of two elements? It seems kind of unnecessary, but then it would be consistent with certificate of is_relatively_complemented().

Last edited 5 years ago by jmantysalo (previous) (diff)

comment:10 in reply to: ↑ 9 ; follow-up: Changed 5 years ago by tscrim

Replying to jmantysalo:

Travis, I reformatted the input-output -blocks like in #20972 (btw, thanks for review).

Doc LGTM.

But should the certificate be one object, i.e. a tuple of two elements? It seems kind of unnecessary, but then it would be consistent with certificate of is_relatively_complemented().

I would have the certificate be one object. It couples the output together and allows the user to have a more standard API of result, cert = P.certificate_returning_method().

Kevin, do you have the rest of the review?

comment:11 Changed 5 years ago by kdilks

Yeah, I should be able to get this reviewed today.

comment:12 Changed 5 years ago by git

  • Commit changed from 8f1c6694856986261bf6b7d90622d65079b12e17 to abfcae83def761c57b61313335c0ce0ded864457

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

abfcae8Make certificate a one object.

comment:13 in reply to: ↑ 10 Changed 5 years ago by jmantysalo

  • Status changed from needs_info to needs_review

Replying to tscrim:

I would have the certificate be one object. It couples the output together and allows the user to have a more standard API of result, cert = P.certificate_returning_method().

OK, done that.

Warning to reviewer: last patch is blind-coded as I work in a very slow machine now.

(After all these *complememt*-function we should think about SEEALSO-blocks. But for now we have at least #20817 still in queue.)

comment:14 follow-up: Changed 5 years ago by kdilks

I guess I misunderstood what was being said about the certificate being a tuple. My presumption was that the function would always return True if true, but would either return False or a tuple with all possibly relevant data if false depending on certificate. Isn't that how other certificates in Sage work?

comment:15 in reply to: ↑ 14 Changed 5 years ago by jmantysalo

Replying to kdilks:

I guess I misunderstood what was being said about the certificate being a tuple. My presumption was that the function would always return True if true, but would either return False or a tuple with all possibly relevant data if false depending on certificate. Isn't that how other certificates in Sage work?

It depends. Certificate for the dimension of a poset is a list of linear extensions and for breadth of a lattice a list of elements.

For is_dismantlable there is both positive and negative certificate: a dismantling order or a crown subposet.

But I think there are also functions so that they always return a tuple whose first element is a Boolean value.

Last edited 5 years ago by jmantysalo (previous) (diff)

comment:16 follow-ups: Changed 5 years ago by kdilks

If it's not particular consistent/canonical, then I'm ok with it the way it is.

Under OUTPUT, I think the two possibilities for certificate should be separate bullet points, and the default possibility (certificate=False) should be listed first.

I also don't especially like the usage of e and e_ as variable names, but don't really have a justifiable reason to make you change it.

Otherwise, it looks good.

comment:17 in reply to: ↑ 16 Changed 5 years ago by tscrim

Replying to kdilks:

Under OUTPUT, I think the two possibilities for certificate should be separate bullet points, and the default possibility (certificate=False) should be listed first.

I disagree. It makes it seems as if there are multiple things in the output (see comment 8 of #20972).

I also don't especially like the usage of e and e_ as variable names, but don't really have a justifiable reason to make you change it.

I don't like the e and e_ either. The first is fine, but the other makes it seem much more correlated with e and is somewhat strange to read. I'd use ep (for e' or e-prime) or f.

comment:18 in reply to: ↑ 16 Changed 5 years ago by jmantysalo

Replying to kdilks:

If it's not particular consistent/canonical, then I'm ok with it the way it is.

Or maybe we are now making the standard. Sometimes there might exists a "yes"-certificate, sometimes "no"-certificate, and sometimes both. Hence we can not have strictly unified interface. Always returning a tuple with Boolean value in first position is most close we can do.

Under OUTPUT, I think the two possibilities for certificate should be separate bullet points, and the default possibility (certificate=False) should be listed first.

Arghs. Whatever, as long as it is consistent.

I also don't especially like the usage of e and e_ as variable names, but don't really have a justifiable reason to make you change it.

I can change those.

comment:19 Changed 5 years ago by jmantysalo

And about more complicated certificates... See #21002.

comment:20 Changed 5 years ago by git

  • Commit changed from abfcae83def761c57b61313335c0ce0ded864457 to 26026128b22eed40d46930a8584355abde69ad8d

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

2602612Variable name from e_ to f.

comment:21 Changed 5 years ago by kdilks

If they can't be separate bullet points, can we at least have the default/simpler/more commonly used option listed first?

It might be a bit more direct to simply combine the information about input and output into the INPUT the way dimension() does

  INPUT:

   * "certificate" (boolean; default:"False") -- whether to return
     an integer (the dimension) or a certificate, i.e. a smallest set
     of linear extensions.

New commits:

2602612Variable name from e_ to f.

comment:22 Changed 5 years ago by jmantysalo

Travis doesn't like it, but I still think that we should close this one and later open a ticket for just to unify docstrings. Maybe after discussion in sage-devel.

comment:23 Changed 5 years ago by tscrim

The description of the certificate is not part of the input, it is the output, so I don't think we should include it in the input. For simple things, this might be okay, but I prefer the separation of concerns. However, I'm not opposed to having the True case first.

comment:24 follow-up: Changed 5 years ago by kdilks

Do you mean having the default certificate=False case first? I'll settle for just that being changed.

comment:25 Changed 5 years ago by git

  • Commit changed from 26026128b22eed40d46930a8584355abde69ad8d to b198ceee80bc66133224027a0ca75a1251d684ea

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

b198ceeMinor doc change.

comment:26 in reply to: ↑ 24 Changed 5 years ago by jmantysalo

Replying to kdilks:

Do you mean having the default certificate=False case first? I'll settle for just that being changed.

Now they are in different order in this and is_relatively_complemented. But OK for me...

Some day after poset documentation polishing I'll run the same for lattices.

comment:27 Changed 5 years ago by kdilks

  • Milestone changed from sage-7.3 to sage-7.4
  • Reviewers set to Kevin Dilks
  • Status changed from needs_review to positive_review

comment:28 Changed 5 years ago by vbraun

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