Opened 5 years ago

Closed 5 years ago

#17552 closed enhancement (fixed)

SPKG for bliss

Reported by: azi Owned by:
Priority: major Milestone: sage-6.5
Component: packages: optional Keywords:
Cc: ncohen Merged in:
Authors: Jernej Azarija Reviewers: Jeroen Demeyer, Nathann Cohen
Report Upstream: N/A Work issues:
Branch: 85719d7 (Commits) Commit: 85719d797c6166e24bd159743c4b2aa2b65618b0
Dependencies: Stopgaps:

Description (last modified by ncohen)

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

Change History (30)

comment:1 Changed 5 years ago by azi

  • Branch set to u/azi/blissSPKG
  • Commit set to eaec62f8c8cd2856dc9761c16975018ac85e42e8
  • Status changed from new to needs_review

New commits:

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

comment:2 Changed 5 years ago by azi

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

comment:3 Changed 5 years ago by ncohen

  • Branch changed from u/azi/blissSPKG to public/17552
  • Commit changed from eaec62f8c8cd2856dc9761c16975018ac85e42e8 to 0e2d04f26fdab0a0313d34ba46637716fcd79e6c
  • Description modified (diff)

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:

343b145trac #17552: Initial attempt at creating a SPKG for bliss
0e2d04ftrac #17552: review

comment:4 Changed 5 years ago by jdemeyer

  • Component changed from PLEASE CHANGE to packages: optional
  • Status changed from needs_review to needs_work
  • Type changed from PLEASE CHANGE to enhancement

Use $MAKE instead of make.

comment:5 Changed 5 years ago by jdemeyer

And quote $SAGE_LOCAL:

mv libbliss.a "$SAGE_LOCAL/lib"

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

comment:6 Changed 5 years ago by git

  • Commit changed from 0e2d04f26fdab0a0313d34ba46637716fcd79e6c to b353d5f8800fec1fdddada1e25a403f13d6633ce

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

b353d5ftrac #17552: Reviewing the review

comment:7 Changed 5 years ago by ncohen

  • Status changed from needs_work to needs_review

comment:8 Changed 5 years ago by jdemeyer

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

comment:9 follow-up: Changed 5 years ago by azi

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.

comment:10 Changed 5 years ago by git

  • Commit changed from b353d5f8800fec1fdddada1e25a403f13d6633ce to 6239deadd908ae0158ae1f63d253ba334b761d98

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

6239deatrac #17552: Added copying of headers to the include dir

comment:11 Changed 5 years ago by azi

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.

comment:12 Changed 5 years ago by ncohen

  • Authors set to Jernej Azarija
  • Reviewers set to Jeroen Demeyer, Nathann Cohen

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

comment:13 in reply to: ↑ 9 ; follow-up: Changed 5 years ago by jdemeyer

Replying to 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?

comment:14 in reply to: ↑ 13 Changed 5 years ago by ncohen

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?

I do not know how to do that if it means using the GMP that is installed by Sage, and declaring GMP as a dependency of bliss. Could you add a commit if you are interested by this feature ?

Nathann

comment:15 follow-up: Changed 5 years ago by azi

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.

Last edited 5 years ago by azi (previous) (diff)

comment:16 Changed 5 years ago by git

  • Commit changed from 6239deadd908ae0158ae1f63d253ba334b761d98 to 3e0caa8c5a3b9aa985641f4b568d4b2eea32b7de

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

48805f5Cython wrapper example
95dcb37trac #17464: First attempt at integrating bliss into sage
50eb67ftrac #17464: Added module_list as well
076a55etrac #17464: few modifications to the bliss wrapper
ebba1aetrac #17464: spliting graph/digraph creating function
3e0caa8trac #17552: Added move of "kqueue.hh" header to local/include

comment:17 Changed 5 years ago by git

  • Commit changed from 3e0caa8c5a3b9aa985641f4b568d4b2eea32b7de to 7cb683c2d153d561e14ad81fe142fc524128e2f1

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

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

comment:18 Changed 5 years ago by ncohen

  • Status changed from needs_review to positive_review

comment:19 Changed 5 years ago by jdemeyer

  • Status changed from positive_review to needs_work

You are not checking the result of $MAKE:

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

comment:20 in reply to: ↑ 15 Changed 5 years ago by jdemeyer

Replying to 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...

comment:21 Changed 5 years ago by jdemeyer

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

comment:22 Changed 5 years ago by git

  • Commit changed from 7cb683c2d153d561e14ad81fe142fc524128e2f1 to 85719d797c6166e24bd159743c4b2aa2b65618b0

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

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

comment:23 follow-up: Changed 5 years ago by azi

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?

comment:24 Changed 5 years ago by azi

  • Status changed from needs_work to needs_review

comment:25 in reply to: ↑ 23 Changed 5 years ago by jdemeyer

Replying to 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.

comment:26 follow-up: Changed 5 years ago by jdemeyer

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

Last edited 5 years ago by jdemeyer (previous) (diff)

comment:27 Changed 5 years ago by git

  • Commit changed from 85719d797c6166e24bd159743c4b2aa2b65618b0 to 9725575a97aed409cfe2c833b24cd5946c23c2a9

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

9725575trac #17552: Added additional error check for make

comment:28 Changed 5 years ago by git

  • Commit changed from 9725575a97aed409cfe2c833b24cd5946c23c2a9 to 85719d797c6166e24bd159743c4b2aa2b65618b0

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

comment:29 in reply to: ↑ 26 Changed 5 years ago by ncohen

  • Status changed from needs_review to positive_review

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

comment:30 Changed 5 years ago by vbraun

  • Branch changed from public/17552 to 85719d797c6166e24bd159743c4b2aa2b65618b0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.