Opened 4 years ago

Closed 4 years ago

## #19392 closed enhancement (fixed)

Reported by: Owned by: cvorland major sage-6.10 combinatorics tscrim, kdilks, jessicapalencia, jmantysalo Corey Vorland Kevin Dilks, Jessica Striker N/A f8fd531 (Commits) f8fd531c64977cdce110889b255e54fa31f70e29

### comment:1 Changed 4 years ago by cvorland

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

New commits:

 ​890d542 Added 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: ↓ 5 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

• 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:

 ​0203f95 Addressed 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:

 ​179b4d7 added 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:

 ​6dbaaaa Addressed 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:

 ​10f7bd2 Addressed 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:

 ​f8fd531 Addressed 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.