Opened 7 years ago

Closed 7 years ago

#13709 closed enhancement (fixed)

Schlafli graph constructor

Reported by: ncohen Owned by: jason, ncohen, rlm
Priority: major Milestone: sage-5.7
Component: graph theory Keywords:
Cc: dcoudert Merged in: sage-5.7.beta4
Authors: Nathann Cohen Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #13510 Stopgaps:

Description

Teaches Sage another graph it did not know. Thanks to Geoff Tims ! :-)

Nathann

Attachments (1)

trac_13709.patch (4.3 KB) - added by ncohen 7 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 7 years ago by ncohen

  • Status changed from new to needs_review

comment:2 Changed 7 years ago by vbraun

  • Dependencies changed from 13510 to #13510

Dependency must start with a hash-character for the patchbot to pick it up

comment:3 follow-up: Changed 7 years ago by vbraun

  • Reviewers set to Volker Braun
  • Status changed from needs_review to positive_review

About half the changesets are unnecessary whitespace changes... Do you see any point besides breaking other people's patches?

Anyways, good enough.

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

About half the changesets are unnecessary whitespace changes... Do you see any point besides breaking other people's patches?

Well.. I have been told to never remove all trailing whitespaces from a patch at once because it broke patch, and to remove them slowly instead, patch by patch. So I remove some when I go over them.

I would prefer to remove them all at once, of course.

Nathann

comment:5 Changed 7 years ago by vbraun

Just don't do it at all! If you even care about trailing whitespace at all (why?) then focus your effort on not introducing extra. Its an OCD ritual that you have to stop ;-)

comment:6 Changed 7 years ago by ncohen

Well. I did not even know they existed, and then the combinat guys said that it was evil :-P

comment:7 Changed 7 years ago by dcoudert

  • Cc dcoudert added

Hello,

just one minor comment about this patch: you use both Shaefli and Schäfli (and I don't know which is the correct one). Are you sure anyone is able to see the second one? No encoding issue? For instance, when I look at the source code of the patch in my browser, the "Schäfli" are replaced with some stupid "SchlÀfli"... When I do graphs.SchaefliGraph??? the output is with "ä" in my terminal. Shouldn't you use only "ae"?

Best.

comment:8 Changed 7 years ago by ncohen

Hmmmm.. Well, it does not seem to be the correct name, so I tried to use "ae" as rarely as possible.. I guess it will not confuse anybody,and this way it appears in google whatever we type in the search bar :-)

Nathann

comment:9 Changed 7 years ago by vbraun

The source file graph_generators.py specifies UTF-8 and the patch is correctly encoded. The trac preview of the patch screws up the encoding. In German, ae is the correct substitute for ä if you are unable to use the umlaut. So it is correct that the Python identifier is Schaefli and the docstring is Schäfli.

comment:10 Changed 7 years ago by dcoudert

OK. It was just to be sure.

comment:11 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.5 to sage-pending

comment:12 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-pending to sage-5.7

comment:13 Changed 7 years ago by jdemeyer

  • Status changed from positive_review to needs_work

This needs to be rebased to sage-5.7.beta2.

Changed 7 years ago by ncohen

comment:14 Changed 7 years ago by ncohen

  • Status changed from needs_work to positive_review

I rebased it on beta1 as beta2 does not work on my machine, but I guess that it'll still pass :-)

Nathann

comment:15 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.7.beta4
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.