Opened 4 years ago

Closed 4 years ago

#19392 closed enhancement (fixed)

Add tetrahedral poset code

Reported by: cvorland Owned by:
Priority: major Milestone: sage-6.10
Component: combinatorics Keywords:
Cc: tscrim, kdilks, jessicapalencia, jmantysalo Merged in:
Authors: Corey Vorland Reviewers: Kevin Dilks, Jessica Striker
Report Upstream: N/A Work issues:
Branch: f8fd531 (Commits) Commit: f8fd531c64977cdce110889b255e54fa31f70e29
Dependencies: Stopgaps:

Description


Change History (20)

comment:1 Changed 4 years ago by cvorland

  • Branch set to u/cvorland/19392
  • Commit set to 890d542c0b16ac87ffb0ea6abe43c16ccd4fbece

New commits:

890d542Added tetrahedral poset to poset_examples.py

comment:2 Changed 4 years ago by cvorland

  • Status changed from new to needs_review

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

  • This needs some tests, and maybe one more example. Examples can be a good way to illustrate an interesting feature of tetrahedral posets, and you can also show how it reduces to perhaps more well-known posets for certain parameters. Also, the code should throw an error if the rest of the input includes something other than one of the six strings.
  • First line of doc should be a single sentence. So I would make the first line be "Returns the tetrahedral poset based on the input colors.", then have a slightly more detailed explanation in a second paragraph with the reference.
  • edge_colors is currently superfluous. Though maybe posets should have an option similar to element_labels where you can assign labels to the covering relations of your poset. It would be useful here, and necessary if anybody ever wanted to try and implement EL-labellings.
  • I would change the last two lines to just return Poset([elem,rels],cover_relations=True).

comment:4 Changed 4 years ago by kdilks

As an aside, it looks like EL labeling is sort of already in Sage, but it just lets you put in a function on pairs of elements that cover each other, and it'll tell you if that defines an EL labelling.

Last edited 4 years ago by kdilks (previous) (diff)

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

  • Cc jmantysalo added

Replying to kdilks:

  • First line of doc should be a single sentence. So I would make the first line be "Returns the tetrahedral poset based on the input colors."

Actually "return", not "returns". And there is a missing space before it. (I.e. an indentation of 3 spaces instead of 4.)

comment:6 Changed 4 years ago by git

  • Commit changed from 890d542c0b16ac87ffb0ea6abe43c16ccd4fbece to 0203f959fda4140c4ce6f5e27ecb73c87df824b2

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

0203f95Addressed reviewers comments, added dictionary for better labeling

comment:7 Changed 4 years ago by git

  • Commit changed from 0203f959fda4140c4ce6f5e27ecb73c87df824b2 to 179b4d7a8fe0017e26108ce8fd40994f3c733129

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

179b4d7added variable for optional integer labeling, edited documentation

comment:8 Changed 4 years ago by jessicapalencia

  • Reviewers changed from Kevin Dilks to Kevin Dilks, Jessica Striker

Looks good. A few comments on the doc:

In the second sentence, the color 'yellow' is missing.

Add 'For example,' to the start of the fourth sentence.

The fourth, fifth, and sixth sentences need periods at the end.

comment:9 Changed 4 years ago by kdilks

Under INPUT:, you should have ``False`` instead of just False.

Try making it "totally self-complementary plane partitons in a `2n \times 2n \times 2n` box".

Description for optional parameter int_label should be more explicit in saying which boolean value gives which labeling.

comment:10 Changed 4 years ago by git

  • Commit changed from 179b4d7a8fe0017e26108ce8fd40994f3c733129 to 6dbaaaae02bc7dcd81348d2980fca043d0be4514

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

6dbaaaaAddressed reviewers comments.

comment:11 Changed 4 years ago by jmantysalo

ordinal_sum() in posets have optional parameter labels with possible values 'pairs' and 'integers'. Should this follow that pattern instead of int_label, as Sage uses int_label nowhere before this? If not, at least try to copy some existing pattern for the parameter.

comment:12 Changed 4 years ago by jessicapalencia

  • Status changed from needs_review to needs_work

One doctest fails (the one with is_isomorphic).

comment:13 Changed 4 years ago by git

  • Commit changed from 6dbaaaae02bc7dcd81348d2980fca043d0be4514 to 10f7bd2d9704da9e86b143ccfd6e004ad1002285

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

10f7bd2Addressed reviewers comments

comment:14 Changed 4 years ago by cvorland

  • Status changed from needs_work to needs_review

comment:15 Changed 4 years ago by kdilks

Just two more minor things

  • Add TetrahedralPoset to the index of methods at the top of the file.
  • I would put labelcount=0 immediately before (and at the same indentation as) for (i,j,k) in elem:. No point in initializing labelcount until you've checked to see that it's going to be used.

comment:16 Changed 4 years ago by git

  • Commit changed from 10f7bd2d9704da9e86b143ccfd6e004ad1002285 to f8fd531c64977cdce110889b255e54fa31f70e29

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

f8fd531Addressed reviewers comments.

comment:17 Changed 4 years ago by kdilks

  • Branch changed from u/cvorland/19392 to u/kdilks/19392

comment:18 Changed 4 years ago by kdilks

  • Branch changed from u/kdilks/19392 to u/cvorland/19392

comment:19 Changed 4 years ago by kdilks

  • Status changed from needs_review to positive_review

comment:20 Changed 4 years ago by vbraun

  • Branch changed from u/cvorland/19392 to f8fd531c64977cdce110889b255e54fa31f70e29
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.