Make bliss return the size of the automorphism group #278
Conversation
Bliss previously returned the size of the automorphism group as a double, which is not useful for GAP. Instead of trying to build the size of the group during search, store a list of numbers which must be multiplied together and return that to GAP.
|
Thanks @ChrisJefferson, I think the sage guys are using their own bliss instead of the one vendored by Digraphs. Agreed that it is annoying to have to tests working in two different cases. Is one possibility to submit a patch to https://github.com/mkoeppe/bliss which is I think where Sage gets its Bliss from. Maybe @nthiery or @embray can offer more details of the rationale behind this if necessary. |
There was a problem hiding this comment.
Thanks a lot @ChrisJefferson.
I've just updated the tests to suppress printing of the the newly-known size. This gets it working, which although not ideal is fine, I think.
|
Anyway, since the tests are passing, I'd be happy for this to be merged. Everyone else agree? In which case I recommend a squash and merge, to get rid of the two commits that I added. |
|
I'm not sure I understand the need for a list. Why not use the gmp class BigNum and convert that to GAP? |
|
Because I'd have to figure out how to get bliss to use the same gmp library as GAP, get all the headers linked together, and also the gmp object in bliss would be allocated with malloc and we'd have to get it moved into gasman memory. I imagine all these things are possible, but I'm not positive how to do them correctly.
Sent from Nine<http://www.9folders.com/>
…________________________________
From: Isuru Fernando <notifications@github.com>
Sent: Thursday, 16 January 2020 22:48
To: gap-packages/Digraphs
Cc: Christopher Jefferson; Mention
Subject: {Disarmed} Re: [gap-packages/Digraphs] Make bliss return the size of the automorphism group (#278)
I'm not sure I understand the need for a list. Why not is the gmp class BigNum and convert that to GAP?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#278>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAGGEB67GJT7XCZE3TMPMSLQ6DP5LANCNFSM4KHTE6LA>.
[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "#278", "url": "#278", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]
|
|
I might just pull in @fingolfin, just to see if he thinks making gmp variables in bliss and then getting them turned in to GAP numbers is reasonable / easy. |
|
I'm okay with you using a list here, but would like it if there's a way for distros to use GMP. |
|
As for GMP, you only need to care about getting the headers correctly. You don't have to link because gap itself will be pulling in GMP before you in import semigroups in GAP. |
|
I'm not fully informed about the context of this and what are possible options and what not; but let me point out that we added If using |
|
I'm also happy with this. |
|
(Just to clarify, I am not objecting to this PR or taking any stance, just replying to @ChrisJefferson's question :-) |
|
@fingolfin : Thanks, I didn't think you were, I was just worried about trying to use GMP "normally" and then trying to get that number into GAP. Personally, I do think this is a bit of a horrible hack, but bliss is basically abandoned at this point. If anyone else wanted to neaten this up for general use, I'd be happy to help do that, but for my purposes this is "good enough". |
* Make bliss return the size of the automorphism group Bliss previously returned the size of the automorphism group as a double, which is not useful for GAP. Instead of trying to build the size of the group during search, store a list of numbers which must be multiplied together and return that to GAP. * cpplint * Fix tests Co-authored-by: Wilf Wilson <wilf@wilf-wilson.net>
Bliss previously returned the size of the automorphism group
as a double, which is not useful for GAP. Instead of trying to
build the size of the group during search, store a list of numbers
which must be multiplied together and return that to GAP.
This PR isn't ready yet (various tests fail). The main problem is it's going to be annoying to make the tests work with, and without, an external bliss. I was wondering why that option is there, and who is using it?