Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#15044 closed enhancement (fixed)

Meredith Graph constructor

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.1
Component: graph theory Keywords:
Cc: Merged in:
Authors: Nathann Cohen Reviewers: Rob Beezer, Volker Braun
Report Upstream: N/A Work issues:
Branch: u/ncohen/15044 (Commits) Commit: f1bf7bb65b605388ab5b1b4e48bb48011ebf0a41
Dependencies: Stopgaps:

Description

http://en.wikipedia.org/wiki/Meredith_graph

Here it is. As my first Git patch :-P

Nathann

Change History (23)

comment:1 Changed 6 years ago by ncohen

  • Status changed from new to needs_review

comment:2 Changed 6 years ago by rbeezer

Alright, Nathann. Two can play this game. My first attempt to review a patch with git. ;-) Went very well, thank-you.

HTML documentation. Meredith Graph as link in 3xN table is not a link. I see it as an input to __append_to_doc. I have tried make doc and various other incantations. Do I need to do something special? This is all I can see that would hold up a positive review.

Very neat graph. Plot is pretty. Factored characteristic polynomial in Sage matches Wikipedia's so I'd say the probability is very high that the graph is correct. ;-)

Rob

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

Hellooooooooooo Rob !

Nicely spotted. I had written "Mederith" instead of "Meredith", and of course it had to fail somewhere :-P

I just updated my branch by changing the last commit instead of commiting a one-line change, which I am told is a dangerous thing to do in general. Doing dangerous things is the best way to learn quickly how it all works :-P

I hope that it will be fine on your side ! :-)

Have fuuuuuuuuuuuuuun !

Nathann

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

  • Commit set to 90b4aa8eb215dd3567fefff2d09576f555c5c370
  • Reviewers set to Rob Beezer
  • Status changed from needs_review to positive_review

Replying to ncohen:

Nicely spotted. I had written "Mederith" instead of "Meredith", and of course it had to fail somewhere :-P

I kept misspelling it while I searched the source, etc, so I should have figured that out! ;-)

I hope that it will be fine on your side ! :-)

All fine on your side. I spent *forever* trying to make it a "tracking" branch on my side. Maybe details on sage-git later today.

All good for a positive review.

I added a commit hash in the "Commit" field - I hope that is right.

Rob

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

Helloooo Rob !!!

All fine on your side. I spent *forever* trying to make it a "tracking" branch on my side. Maybe details on sage-git later today.

Hmmmm... I don't even know what that means yet :-P

All good for a positive review.

Thanks !!

I added a commit hash in the "Commit" field - I hope that is right.

Hmmmmm.. Some time ago I think I saw a discussion on sage-git that I didn't understand at that time about whether we should use branches or commits references. And I think that we should use branches instead, for the following reason :

  • This Meredith patch was uploaded yesterday, when the git version of Sage was still version 5.11.rc0
  • I then updated my version of sage-git to 5.12 with a git pull
  • I now want to write a patch #15049 that is based upon this patch

The point now, is that this patch's commit was created before 5.12. Hence if I want to write #15049 atop this patch, I can't be above 5.12 too. What I could do, however, is rebase this patch atop 5.12, hence updating the u/ncohen/meredith branch, then write #15049 on this new meredith branch which is above 5.12.

The problem is that the hash of a commit changes when you rebase it. Or at least I found no way to avoid that O_o

Sooooooo hoping that it is not a problem, I will try to rebase this patch above 5.12 (and there should not be any conflict), update the commit field to match the new version of the meredith commit, then base #15049 atop of that.

I hope all of that was clear :-P

Have fuuuuuuuuuun ! And thank you again :-)

Nathann

Last edited 6 years ago by ncohen (previous) (diff)

comment:6 Changed 6 years ago by ncohen

  • Commit changed from 90b4aa8eb215dd3567fefff2d09576f555c5c370 to 2023ac74ff65df6a92f962c70c4d058a0722e6c9

comment:7 Changed 6 years ago by jdemeyer

  • Authors set to Nathann Cohen
  • Milestone changed from sage-5.12 to sage-6.0

comment:8 follow-up: Changed 6 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Did you mean # long time instead of # long?

comment:9 in reply to: ↑ 8 Changed 6 years ago by ncohen

  • Commit 2023ac74ff65df6a92f962c70c4d058a0722e6c9 deleted

Did you mean # long time instead of # long?

Right. I just replaced it with a "long time".

And because I had to make this small new commit, and because I merged it with this Meredith patch, I had to rebase and reupload #15049. Because git remembers the hash of its dependencies, and so even if the small modifications that you make do not create any conflict you have to rebase everything above T_T

This is going to be painful in the very near future O_O

Nathann

comment:10 Changed 6 years ago by ncohen

  • Status changed from needs_work to positive_review

comment:11 Changed 6 years ago by ncohen

  • Branch changed from u/ncohen/meredith_graph to u/ncohen/15044

(just changing the branch's name)

Nathann

comment:12 Changed 6 years ago by git

  • Commit set to cc9d0c3c814ffcd464765b0d24ff835062bef587
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

[changeset:cc9d0c3]Merge branch 'start' into meredith
[changeset:ac53c3d]Meredith Graph constructor

comment:13 Changed 6 years ago by ncohen

I just rebased it... I hope it was effective :-/

Nathann

comment:14 Changed 6 years ago by kcrisman

How do we review things NOT using git that are in git? I have some motivation to review #15054 (a friend who really loves snarks and needs to be encourage to use Sage to explore them) but am not going to have time to learn the new workflow yet (despite Volker's excellent slides from the Sage Days this week).

comment:15 Changed 6 years ago by git

  • Commit changed from cc9d0c3c814ffcd464765b0d24ff835062bef587 to d66b7d10231d055a4dda0b11c9ee3dfdb2a9f79f

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

[changeset:d66b7d1]Meredith Graph constructor
[changeset:3b15578]Merging Sage-5.12.beta5, newest dev scripts, and the doctest fixes.
[changeset:1456c52]Merge branch 'ticket/14482' into public/sage-git/master
[changeset:b890215]Merge branch 'ticket/14482' into public/sage-git/master
[changeset:d8713eb]Merge remote-tracking branch 'origin/build_system' into public/sage-git/master
[changeset:970090d]Merge branch 'u/ohanar/build_system'
[changeset:cf14c84]Merge branch 'ticket/14482' into public/sage-git/master
[changeset:060184c]Merge branch 'ticket/14482' into public/sage-git/master
[changeset:b95a820]Merge branch 'ticket/14482' into public/sage-git/master
[changeset:a10bc4f]Merge branch 'ticket/14482'

comment:16 Changed 6 years ago by ncohen

Just rebased it. Seems better this way :-P

Nathann

comment:17 Changed 6 years ago by rbeezer

Hellooooooooooo Nathann,

I have not forgotten this one. I am learning git on some smaller scale projects, and I'm close to trying this one again. I *will* be back, and better prepared.

Rob

comment:18 Changed 6 years ago by ncohen

Thaaaaaaanks :-D

Nathann

comment:19 Changed 6 years ago by git

  • Commit changed from d66b7d10231d055a4dda0b11c9ee3dfdb2a9f79f to f1bf7bb65b605388ab5b1b4e48bb48011ebf0a41

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

[changeset:f1bf7bb]Rebase on 5.13.beta0
[changeset:d66b7d1]Meredith Graph constructor
[changeset:3b15578]Merging Sage-5.12.beta5, newest dev scripts, and the doctest fixes.
[changeset:1456c52]Merge branch 'ticket/14482' into public/sage-git/master
[changeset:b890215]Merge branch 'ticket/14482' into public/sage-git/master
[changeset:d8713eb]Merge remote-tracking branch 'origin/build_system' into public/sage-git/master
[changeset:970090d]Merge branch 'u/ohanar/build_system'
[changeset:cf14c84]Merge branch 'ticket/14482' into public/sage-git/master
[changeset:060184c]Merge branch 'ticket/14482' into public/sage-git/master
[changeset:b95a820]Merge branch 'ticket/14482' into public/sage-git/master

comment:20 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.0 to sage-6.1

comment:21 Changed 6 years ago by vbraun

  • Reviewers changed from Rob Beezer to Rob Beezer, Volker Braun
  • Status changed from needs_review to positive_review

lgtm

comment:22 Changed 6 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:23 Changed 6 years ago by ncohen

Thaaaaaaaaaaaanks !! ;-)

Nathann

Note: See TracTickets for help on using tickets.