Opened 9 years ago

Last modified 9 years ago

#14618 closed enhancement

Add generators for fullerenes — at Version 29

Reported by: nvcleemp Owned by: jason, ncohen, rlm
Priority: major Milestone: sage-5.11
Component: packages: optional Keywords:
Cc: azi, Slani, Stefan, ncohen Merged in:
Authors: Nico Van Cleemput Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #14532 Stopgaps:

Status badges

Description (last modified by ncohen)

In the discussion of ticket #9136, it was mentioned that a generator for fullerenes would be a great addition. The fullerene generator fullgen by Gunnar Brinkmann can't be added due to licensing problems. Brinkmann's student Jan Goedgebeur implemented a new version using a different algorithm which is faster for the `small' cases: http://caagt.ugent.be/buckygen/ This program is available under the GPL 3, so I assume it can be integrated in Sage.

New spkg: http://users.ugent.be/~nvcleemp/buckygen-1.0.spkg

Apply:

Change History (30)

comment:1 Changed 9 years ago by nvcleemp

I'm willing to work on this. I belong to the same research group and have some familiarity with the program and the used file formats, since I integrated it into CaGe (http://caagt.ugent.be/CaGe). I'm quite new to Sage, so I might however need some tips as to how to go about integrating it into Sage.

comment:2 follow-up: Changed 9 years ago by ncohen

This would be a great addition to Sage ! this being said, a bad license does not necessarily mean that the software will never be used through Sage : it just means that it cannot be distributed with Sage. But we can use Nauty, which is not GPL-compatible, through the Nauty spkg :-)

Nathann

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

Indeed that would be nice!

Let us know if you have any questions about integrating this into Sage.

comment:4 in reply to: ↑ 2 ; follow-up: Changed 9 years ago by nvcleemp

Replying to ncohen:

This would be a great addition to Sage ! this being said, a bad license does not necessarily mean that the software will never be used through Sage : it just means that it cannot be distributed with Sage. But we can use Nauty, which is not GPL-compatible, through the Nauty spkg :-)

Nathann

Yes, but for most cases buckygen outperforms fullgen, so it is the better choice. This is also the reason why it is now used in CaGe, except when the fullerenes need to be filtered based on the symmetry group or when they are needed in a specific order. These are all arguments which are mostly important for chemists, and even for them only in specific situations.

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

Yooooooo !

Yes, but for most cases buckygen outperforms fullgen, so it is the better choice.

Oh. Well, that's even better :-P

Nathann

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

Replying to azi:

Indeed that would be nice!

Let us know if you have any questions about integrating this into Sage.

Well, buckygen supports export in graph6, sparse6 and planar_code format. Using the first two is no problem, since they are already supported by Sage. However, this looses some information: buckygen generates plane graphs. This doesn't seem to be supported in Sage. Is that correct? Is it something that might be added in the near future?

Secondly, I had a look at the geng generator to get an idea how to integrate this kind of functionality. The Sage-part seems rather straight-forward. What I do have a question about, is how to add the buckygen program and make sure that it gets compiled, is available, ...

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

Well, buckygen supports export in graph6, sparse6 and planar_code format. Using the first two is no problem, since they are already supported by Sage. However, this looses some information: buckygen generates plane graphs. This doesn't seem to be supported in Sage. Is that correct? Is it something that might be added in the near future?

"Planar graphs are not supported" ? What do you mean ? Do you mean that the embedding, or the positions are lost ? May it be what you need ?

http://www.sagemath.org/doc/reference/graphs/sage/graphs/generic_graph.html#sage.graphs.generic_graph.GenericGraph.get_embedding

I never used it myself, though it looks "useful" if you really, really work on planar graphs :-)

Secondly, I had a look at the geng generator to get an idea how to integrate this kind of functionality. The Sage-part seems rather straight-forward. What I do have a question about, is how to add the buckygen program and make sure that it gets compiled, is available, ...

You have to build a spkg.

http://www.sagemath.org/doc/developer/producing_spkgs.html

Nathann

comment:8 follow-up: Changed 9 years ago by nvcleemp

OK, thanks. That seems to answer my questions for now. I know what to do for the next couple of days. ;-)

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

Replying to nvcleemp:

OK, thanks. That seems to answer my questions for now. I know what to do for the next couple of days. ;-)

So, I think I have a first version ready. You will need the following spkg:

http://users.ugent.be/~nvcleemp/buckygen-1.0.spkg

I guess this can become a standard package, since it is GPL v3.

I looked at different spkg and copied what they did, so I hope that everything is in order. The same goes for the patch. Let me know what needs to be changed.

comment:10 Changed 9 years ago by nvcleemp

  • Status changed from new to needs_review

comment:11 Changed 9 years ago by nvcleemp

Ow, just saw that I completely forgot the error handling: I still need to replace those passes. I'm sorry.

comment:12 Changed 9 years ago by nvcleemp

  • Status changed from needs_review to needs_work

comment:13 Changed 9 years ago by nvcleemp

  • Status changed from needs_work to needs_review

OK, errors are now handled.

comment:14 Changed 9 years ago by ncohen

  • Status changed from needs_review to needs_work

Helloooooooooooooooooooooooooooooooooooo !!!

NIiiiiiiice to have this in Sage ! I already know a couple of persons who will be interested in this feature :-)

Some comments:

  • I think that the method should be renamed "fullerenes". Or "fullerenes_buckygen" if you must, but really I think that it has to indicate what it does. And "fullerenes" is pretty nice in this attempt. Of course do cite buckygen wherever you can, it's not made to steal credit or anything, it's just that if I wanted to generate fullerenes I wouldn't know that this is what "buckygen" does.
  • In sage we usually use long names ... But it's true that isolated_pentagon_rule = True is really long :-/
  • Here is what happens when the spkg is not installed
    sage: list(graphs.buckygen(60))
    ---------------------------------------------------------------------------
    AssertionError                            Traceback (most recent call last)
    <ipython-input-2-b82c592bf78f> in <module>()
    ----> 1 list(graphs.buckygen(Integer(60)))
    
    /home/ncohen/.Sage/local/lib/python2.7/site-packages/sage/graphs/graph_generators.pyc in buckygen(self, order, ipr)
       1006         header = out.read(13)
       1007 
    -> 1008         assert header == '>>planar_code', 'Not a valid planar code header'
       1009 
       1010         c = out.read(1)
    
    AssertionError: Not a valid planar code header
    

Could you add a message saying something like "To use this method you must install the buckygen spkg" ?

  • Your doctests do not pass when buckygen is not installed, so you should flag them with # optional - buckygen. You can see how it is done in
    sage: graphs.nauty_geng?
    
  • For LaTeX expression in Sphinx (Sage's documentation builder) you can't use $, and you should use instead. See Graph.dominating_set?` for an example. You can rebuild the documentation (and check that it works) by running this
    sage -b && sage -docbuild reference/graphs html
    
  • There is a special syntax for references in Sphinx. Take a look at sage.graphs.comparability??
  • When n < 20, why don't we just yield an empty iterator ? Is there any reason to raise an exception in this case ?

That's all I can think of right now :-)

And thank you very much again for adding this. That's good stuff !

Nathann

comment:15 Changed 9 years ago by nvcleemp

OK, I'll change all of that. I used ipr, because also in the literature this is almost always referred to as IPR and hardly ever as Isolated Pentagon Rule.

The naming buckygen was only based on nauty_geng: it's certainly no problem to rename it to fullerenes. Btw, that's the way we do it in CaGe?.

comment:16 Changed 9 years ago by ncohen

Well, if it is that standard then it is probably better to stick with "ipr". The alternative is really very long.

And I don't think that nauty_geng is such a good name either, but it has been like that since before I first used Sage, and I don't have a great idea of what it could be replaced with either ^^;

Nathann

comment:17 Changed 9 years ago by nvcleemp

Btw, since buckygen is GPL, is there a reason why the spkg can't be included as a standard spkg?

comment:18 Changed 9 years ago by ncohen

I believe that it has to be optional for a while... But then again it's pretty small. Could you ask on sage-devel ?

By the way, I forgot to add a couple of things to my earlier review : could you remove the patches/ directory from your spkg ? It's not needed unless it is nonempty :-)

Oh, and SPKG.txt file has the +x flag for no reason.

Nathann

comment:19 Changed 9 years ago by nvcleemp

  • Status changed from needs_work to needs_review

Hi, thanks for the feedback. I've modified the spkg, it is still at

http://users.ugent.be/~nvcleemp/buckygen-1.0.spkg

I've indeed found the explanation that a spkg should first be optional for a while on the wiki. I had missed that at first.

I'll also add a new version of the patch.

comment:20 Changed 9 years ago by ncohen

  • Dependencies set to #14532

(I just rebased your patch on Sage 5.10.beta4, as it did not apply cleanly. #14532 was to blame)

comment:21 Changed 9 years ago by nvcleemp

Hmm, ok, what does that mean for me? Anything I need to do? Apply those patches and then remake my patch?

comment:22 Changed 9 years ago by nvcleemp

ok, I'm blind: I didn't see that you've already done that. :-)

comment:23 Changed 9 years ago by ncohen

Nonono. I did that for you. But it means that right now you own patch will not apply cleanly on your own version of Sage anymore. And because I will probably write a small reviewer's patch, you will not be able to apply this on your version of Sage either, unless you add the dependencies yourself.

If you don't need your cpu for a couple of hours, then it's probably the right time to download and compile sage 5.10.beta4.

Have fuuuuuuuuuuuuuuuuuuuuuuuuunnn !!

Nathann

comment:24 Changed 9 years ago by nvcleemp

Since I wrote most of the original patch, I assume I can't review this? ;-)

comment:25 Changed 9 years ago by ncohen

Ahahah. No you can't and you should not. I intended to write a reviewer's patch for this ticket but I had some problems with my copy of Sage, and it was delayed. You will have it by today !

Sorry for that !

Nathann

comment:26 Changed 9 years ago by nvcleemp

hehe, no problem. I was just wondering if there was anything more I needed to do. But I guess I can just lay back and relax. Or maybe start marking the projects of my students. :-(

No hurry!

Changed 9 years ago by ncohen

comment:27 Changed 9 years ago by ncohen

(new version of the original patch without trailing whitespaces)

comment:28 Changed 9 years ago by nvcleemp

hmm, I just noted a small typo on line 990: 20 should be removed there. Currently not at my own computer, so I can't fix it at the moment.

comment:29 Changed 9 years ago by ncohen

  • Description modified (diff)
  • Reviewers set to Nathann Cohen

Heeeeeeeeeeere it is !

This patch touches small things, but mostly simplifies code that could be simplified, and adds some doc. If you agree with it you can set this ticket to positive_review, otherwise let's talk about it :-)

Thank you again for this patch !

Nathann

Last edited 9 years ago by ncohen (previous) (diff)
Note: See TracTickets for help on using tickets.