Opened 5 years ago

Closed 5 years ago

#18959 closed enhancement (fixed)

Poset documentation polishing: integer-valued properties

Reported by: jmantysalo Owned by:
Priority: minor Milestone: sage-6.9
Component: documentation Keywords:
Cc: ncohen, vdelecroix Merged in:
Authors: Jori Mäntysalo Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: 3302f91 (Commits) Commit: 3302f91538b7258d2ad58d413c84a69e0308a53c
Dependencies: Stopgaps:

Description (last modified by jmantysalo)

Check documentation in functions like cardinality(), height() and dimension().

This continues the series beginning with #18925 and #18941.

(Also adds input checking for random_order_ideal().)

Change History (14)

comment:1 Changed 5 years ago by jmantysalo

  • Branch set to u/jmantysalo/poset_documentation_polishing__integer_valued_properties

comment:2 Changed 5 years ago by jmantysalo

  • Authors set to Jori Mäntysalo
  • Cc ncohen added
  • Commit set to 1a21f1f475656fa62525f4acef7790c0857bd963

Code changes: Return type of all functions is now Integer. There was slight inconsistency between for example dimension() and width().

Tests: Added test for empty poset to those functions where it wasn't already.

Docstrings: Added some some kind of "human-readable" explanation to dimension(). Otherwise normal things like "returns" -> "return".

Examples: Tried to make simple examples that are possible to verify by hand. For example relations_number for the Pentagon Poset. For dimension() selected a beatiful poset (with untrivial dimension). :=)

Questions:

Is it exactly true that "Relations are also often called intervals."? I would say that every relation (i.e. pair of comparable elemenst) *defines* an interval.

Is the term 'realizer' commonly used? If so, it should be mentioned in the documentation of dimension() (because of googling).

There is one 'not tested' in dimension(). Can a test combine 'long time' and optional? As it takes less than 30 seconds with additional package, it could be tested with --long.

About English, should one say "the longest chain" or "a longest chain" when there can be several chains equal in lenght? (Compare to "a smallest set of linear extensions" on dimension().)


New commits:

1a21f1fSome polishing to integer-valued poset properties.

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

  • Cc vdelecroix added

Vincent will probably be interested by you int->Integer change.

comment:4 in reply to: ↑ 3 ; follow-up: Changed 5 years ago by jmantysalo

Replying to ncohen:

Vincent will probably be interested by you int->Integer change.

OK. I am not sure what way this should be. For now return type of poset cardinality() differs from type of graph order().

comment:5 in reply to: ↑ 4 ; follow-up: Changed 5 years ago by ncohen

OK. I am not sure what way this should be. For now return type of poset cardinality() differs from type of graph order().

This inconsistency was probably introduced by #18159.

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

Replying to ncohen:

OK. I am not sure what way this should be. For now return type of poset cardinality() differs from type of graph order().

This inconsistency was probably introduced by #18159.

OK, so cardinality() can be seen as an exception. But it does not explain why width() and height() gives different return types.

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

There is ongoing discussion about int vs. Integer in both sage-devel and in another ticket. Nathann, just to make sure: I guess you have nothing against changing return type of height() and dimension()?

You might be interested in other changes too, as I am touching functions made by you.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 5 years ago by ncohen

There is ongoing discussion about int vs. Integer in both sage-devel and in another ticket. Nathann, just to make sure: I guess you have nothing against changing return type of height() and dimension()?

You guess wrong. I view it as the progressive suppression of 'integers' from Sage, for absolutely no good reason. It will make us import Integer everywhere, and wrap infinitely many 'return x' with 'return Integer(x)', just because people cannot stand that Sage's integer is not the default one everywhere.

There is a discussion on #18159 on the theme "I don't mind your returning Sage Integers if you like, but I don't see why you want to force me to do it too".

Nathann

comment:9 Changed 5 years ago by git

  • Commit changed from 1a21f1f475656fa62525f4acef7790c0857bd963 to 2b190b2de01c21578efefe5d876f83e330a12197

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

2b190b2Integer->int, minor other changes.

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

  • Status changed from new to needs_review

Replying to ncohen:

There is ongoing discussion about int vs. Integer in both sage-devel and in another ticket. Nathann, just to make sure: I guess you have nothing against changing return type of height() and dimension()?

You guess wrong.

OK, here is modified patch; int vs. Integer can be resolved later.

This patch contains non-relating modification to random_order_ideal(): direction='junk' will now raise exception instead of behaving like direction='up'.

comment:11 follow-up: Changed 5 years ago by ncohen

  • Reviewers set to Nathann Cohen

Hello !

Two comments:

  • There is an 'Integer(0)' left
  • It is (slightly) better to have # random than # not tested: in the first case the command is not run, while in the second it is run and the output is not compared with the expected results (which would still report an error in case of an exception). Given the command in question ('sage: L') it hardly matters here

Regardless of what you decide to do with those two points, you can set the ticket to positive_review on my behalf.

Nathann

comment:12 Changed 5 years ago by git

  • Commit changed from 2b190b2de01c21578efefe5d876f83e330a12197 to 3302f91538b7258d2ad58d413c84a69e0308a53c

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

3302f91Forgotten Integer(0)

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

  • Description modified (diff)
  • Status changed from needs_review to positive_review

Replying to ncohen:

  • It is (slightly) better to have # random than # not tested: in the first case the command is not run - -

True. Corrected.

  • There is an 'Integer(0)' left

Regardless of what you decide to do with those two points, you can set the ticket to positive_review on my behalf.

I would have said this a bug. I corrected it also.

Thanks!

comment:14 Changed 5 years ago by vbraun

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