SPKG for bliss

[sagetrac-azi]

This ticket adds support for a bliss spkg, available at: http://www.steinertriples.fr/ncohen/tmp/bliss-0.72.tar.bz2

CC: @nathanncohen

Component: packages: optional

Author: Jernej Azarija

Branch/Commit: 85719d7

Reviewer: Jeroen Demeyer, Nathann Cohen

Issue created by migration from https://trac.sagemath.org/ticket/17552

[sagetrac-azi]

Commit: eaec62f

[sagetrac-azi]

New commits:

eaec62f

trac #17552: Initial attempt at creating a SPKG for bliss

[sagetrac-azi]

Branch: u/azi/blissSPKG

[sagetrac-azi]

comment:2

Let me instantly mention that I have no idea how to test if this thing actually works. How is this usually done?

[nathanncohen]

Changed commit from eaec62f to 0e2d04f

[nathanncohen]

comment:3

Yooooooooooooo !

I fixed a couple of things, like long lines and the changelog (we shouldn't have a changelog anymore in those files. It was only useful in the Mercurial era).

Some other modifications later, the branch seems to work:

1. Checkout the branch (which I rebased on the latest beta)
2. Download the bliss archive in your "upsream/" folder
3. Run "sage -i bliss"

http://www.steinertriples.fr/ncohen/tmp/bliss-0.72.tar.bz2

Now that you have a working example, perhaps it will be easier to understand. Also, perhaps you can link your bliss patch without any problem once this is installed ;-)

Nathann

---

New commits:

343b145	trac #17552: Initial attempt at creating a SPKG for bliss
0e2d04f	trac #17552: review

[nathanncohen]

Changed branch from u/azi/blissSPKG to public/17552

[jdemeyer]

comment:4

Use $MAKE instead of make.

[jdemeyer]

comment:5

And quote $SAGE_LOCAL:

mv libbliss.a "$SAGE_LOCAL/lib"

(funny that you quote "src" where it is not needed)

[sagetrac-git]

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

b353d5f

trac #17552: Reviewing the review

[sagetrac-git]

Changed commit from 0e2d04f to b353d5f

[jdemeyer]

comment:8

It seems that bliss supports gmp but this is not compiled in. Any reason?

[sagetrac-azi]

comment:9

Yes, if this option is enabled then bliss can compute the order of the automorphism group. By default this option is not enabled in bliss.

As far as I am concerned I don't think we need this feature since the performance crucial part is to get the generators of the group and once we have them PermuationGroup.order does a pretty good job.

[sagetrac-git]

Changed commit from b353d5f to 6239dea

[sagetrac-git]

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

6239dea

trac #17552: Added copying of headers to the include dir

[sagetrac-azi]

comment:11

I am not a SPKG expert but as far as I've checked (making sure the spkg is installed and all the components are available to sage) this branch looks OK to me.

I've just added a final line to make sure the headers are copied as well and as far as I am concerned that does it for this ticket.

[nathanncohen]

comment:12

Yoooooooo !

Well, I don't see anything wrong with it either. Jeroen, is that okay for you too ? Then we can get this in and Jernej will be able to continue the "real" patch, i.e. the one that uses the spkg.

Nathann

[nathanncohen]

Reviewer: Jeroen Demeyer, Nathann Cohen

[nathanncohen]

Author: Jernej Azarija

[jdemeyer]

comment:13

Replying to @sagetrac-azi:

I don't think we need this feature

I find that a very weak reason to disable GMP support. Perhaps somebody else really cares about that feature. If it doesn't break anything, why not enable it?

[sagetrac-azi]

comment:15

Well the motivation for this patch is the work that I am doing in which I've found that computing the automorphism group of a graph in Sage is quite inefficient. The order itself is never a big computational problem.

That said I'd like this to be as efficient as possible and hence avoid computing the order of the group and just return the generators as quickly as possible.

Hence I would suggest we leave this as is for now and if someone starts to struggle computing orders of the automorphism group of a graph we can easily include it at that point.

My comment "i don't think we need this feature" is based on the fact that I work with graphs in Sage on a daily basis and I honestly think this is not useful at this point.

[sagetrac-git]

Changed commit from 6239dea to 3e0caa8

[sagetrac-git]

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

48805f5	Cython wrapper example
95dcb37	trac #17464: First attempt at integrating bliss into sage
50eb67f	trac #17464: Added module_list as well
076a55e	trac #17464: few modifications to the bliss wrapper
ebba1ae	trac #17464: spliting graph/digraph creating function
3e0caa8	trac #17552: Added move of "kqueue.hh" header to local/include

[sagetrac-git]

Changed commit from 3e0caa8 to 7cb683c

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

7cb683c

trac #17552: Added move of "kqueue.hh" header to local/include

[jdemeyer]

comment:19

You are not checking the result of $MAKE:

cd "src" &&
$MAKE &&
mv libbliss.a "$SAGE_LOCAL/lib"
# (missing check here)

[jdemeyer]

comment:20

Replying to @sagetrac-azi:

My comment "i don't think we need this feature" is based on the fact that I work with graphs in Sage on a daily basis and I honestly think this is not useful at this point.

I get your point but ---like I already said--- I don't think that's a good reason to disable GMP support.

Note: for me, this isn't a "needs_work" issue, I'm just commenting...

[jdemeyer]

comment:21

Alternatively to adding manual checks, you could also use set -e.

[sagetrac-git]

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

85719d7

trac #17552: Introduced bliss patch and made the additional error checkings

[sagetrac-git]

Changed commit from 7cb683c to 85719d7

[sagetrac-azi]

comment:23

I have added a patch to this SPKG fixing a newly found bug in bliss.

As for the error check. Is it actually necessary given that && is used to glue the statements together?

[jdemeyer]

comment:25

Replying to @sagetrac-azi:

Is it actually necessary given that && is used to glue the statements together?

When you do

A && B && C

D

a check is needed between A && B && C and D.

[jdemeyer]

comment:26

The checking is fine now, I'll let Nathann judge the patch.

[sagetrac-git]

Changed commit from 85719d7 to 9725575

[sagetrac-git]

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

9725575

trac #17552: Added additional error check for make

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

[sagetrac-git]

Changed commit from 9725575 to 85719d7

[nathanncohen]

comment:29

The checking is fine now, I'll let Nathann judge the patch.

A commit was added then removed in the meantime, and as a result I have nothing new to mention. Seems to do the job (well, we'll see that in #17464 :-P) and all 'formalities' have been fulfilled as far as I can tell... I switch it to positive_review.

Nathann

[vbraun]

Changed branch from public/17552 to 85719d7