Opened 6 years ago
Closed 6 years ago
#17552 closed enhancement (fixed)
SPKG for bliss
Reported by:  azi  Owned by:  

Priority:  major  Milestone:  sage6.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 )
This ticket adds support for a bliss spkg, available at: http://www.steinertriples.fr/ncohen/tmp/bliss0.72.tar.bz2
Change History (30)
comment:1 Changed 6 years ago by
 Branch set to u/azi/blissSPKG
 Commit set to eaec62f8c8cd2856dc9761c16975018ac85e42e8
 Status changed from new to needs_review
comment:2 Changed 6 years ago by
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 6 years ago by
 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/bliss0.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

comment:4 Changed 6 years ago by
 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 6 years ago by
And quote $SAGE_LOCAL
:
mv libbliss.a "$SAGE_LOCAL/lib"
(funny that you quote "src"
where it is not needed)
comment:6 Changed 6 years ago by
 Commit changed from 0e2d04f26fdab0a0313d34ba46637716fcd79e6c to b353d5f8800fec1fdddada1e25a403f13d6633ce
Branch pushed to git repo; I updated commit sha1. New commits:
b353d5f  trac #17552: Reviewing the review

comment:7 Changed 6 years ago by
 Status changed from needs_work to needs_review
comment:8 Changed 6 years ago by
It seems that bliss supports gmp but this is not compiled in. Any reason?
comment:9 followup: ↓ 13 Changed 6 years ago by
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 6 years ago by
 Commit changed from b353d5f8800fec1fdddada1e25a403f13d6633ce to 6239deadd908ae0158ae1f63d253ba334b761d98
Branch pushed to git repo; I updated commit sha1. New commits:
6239dea  trac #17552: Added copying of headers to the include dir

comment:11 Changed 6 years ago by
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 6 years ago by
 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 ; followup: ↓ 14 Changed 6 years ago by
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 6 years ago by
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 followup: ↓ 20 Changed 6 years ago by
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.
comment:16 Changed 6 years ago by
 Commit changed from 6239deadd908ae0158ae1f63d253ba334b761d98 to 3e0caa8c5a3b9aa985641f4b568d4b2eea32b7de
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

comment:17 Changed 6 years ago by
 Commit changed from 3e0caa8c5a3b9aa985641f4b568d4b2eea32b7de to 7cb683c2d153d561e14ad81fe142fc524128e2f1
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

comment:18 Changed 6 years ago by
 Status changed from needs_review to positive_review
comment:19 Changed 6 years ago by
 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 6 years ago by
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 6 years ago by
Alternatively to adding manual checks, you could also use set e
.
comment:22 Changed 6 years ago by
 Commit changed from 7cb683c2d153d561e14ad81fe142fc524128e2f1 to 85719d797c6166e24bd159743c4b2aa2b65618b0
Branch pushed to git repo; I updated commit sha1. New commits:
85719d7  trac #17552: Introduced bliss patch and made the additional error checkings

comment:23 followup: ↓ 25 Changed 6 years ago by
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 6 years ago by
 Status changed from needs_work to needs_review
comment:25 in reply to: ↑ 23 Changed 6 years ago by
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 followup: ↓ 29 Changed 6 years ago by
The checking is fine now, I'll let Nathann judge the patch.
comment:27 Changed 6 years ago by
 Commit changed from 85719d797c6166e24bd159743c4b2aa2b65618b0 to 9725575a97aed409cfe2c833b24cd5946c23c2a9
Branch pushed to git repo; I updated commit sha1. New commits:
9725575  trac #17552: Added additional error check for make

comment:28 Changed 6 years ago by
 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 6 years ago by
 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 6 years ago by
 Branch changed from public/17552 to 85719d797c6166e24bd159743c4b2aa2b65618b0
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
trac #17552: Initial attempt at creating a SPKG for bliss