Opened 9 years ago

Closed 7 years ago

#11395 closed enhancement (fixed)

Upgrade Gfan to latest release (version 0.5)

Reported by: mhampton Owned by: tbd
Priority: major Milestone: sage-5.5
Component: packages: standard Keywords: tropical geometry
Cc: Merged in: sage-5.5.beta1
Authors: Marshall Hampton, François Bissey Reviewers: François Bissey, Volker Braun
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by vbraun)

From the Gfan page: "Gfan version 0.5 (January 23th 2011, major update, Gröbner bases/fans over the integers (experimental), p-adic Gröbner bases/complexes (experimental), fan homology computations, exporting of fans in Polymake's new xml format, performance improvements (gfan is now faster than TOPCOM for secondary fan computations) and much more.) Updated January 25th 2011 (xml format and installation instructions). "

We should upgrade to this release.

New spkg available at http://spkg-upload.googlecode.com/files/gfan-0.5.p0.spkg

Apply:

Attachments (3)

trac11395-fix_tutorial.patch (3.9 KB) - added by fbissey 7 years ago.
Fix tutorial with gfan 0.5
trac_11395_doctest_fix.patch (1.1 KB) - added by vbraun 7 years ago.
Initial patch
trac_11395_update_gfan_vb.patch (26.8 KB) - added by vbraun 7 years ago.
Added commit message

Download all attachments as: .zip

Change History (35)

comment:1 Changed 9 years ago by cschwan

On Gentoo we already did this, so here a little summary what to do:

  • in sage/interfaces/gfan.py, we have to change cmd = 'gfan' to cmd = 'gfan_bases' because the earlier is deprecated. If we do not do this, gfan prints:

This is the Gfan program for computing Groebner fans and tropical varieties. Use the command "gfan list" to view all subcommands. The command "gfan" is deprecate for computing all Groebner bases of an ideal. Please use subcommand "gfan _bases" instead. Awaiting input. <Ctrl>-D to end.

This confuses the current error detection code which assumes every output before commands as an error.

  • after adjusting the interface code, we have to review the failing doctests. On my box these are:
    • sage -t -force_lib devel/sage-main/doc/fr/tutorial/tour_advanced.rst
    • sage -t -force_lib devel/sage-main/doc/en/tutorial/tour_advanced.rst
    • sage -t -force_lib devel/sage-main/sage/rings/polynomial/groebner_fan.py

I did not have a closer look at them, but they look like gfan-0.5 reorders its output.

comment:2 Changed 9 years ago by mhampton

A candidate package is at http://sage.math.washington.edu/home/mhampton/gfan-0.5.spkg

I will take your suggestion on patching the interface. I am aware of some other issues that need to be fixed; I was going to put those on another ticket but if 0.5 changes its output anyway I suppose it makes sense to put both the update and interface patches in one ticket.

comment:3 Changed 9 years ago by fbissey

I had kind of started work on that. I am starting to hate when sage-on-gentoo is too much ahead and it breaks tests in non trivial ways. Most of the tests mentioned by Christopher are broken by trivial changes of formatting or ordering. But that one I think needs careful checking:

**********************************************************************                                                              
File "/usr/share/sage/devel/sage-main/sage/rings/polynomial/groebner_fan.py", line 606:                                             
    sage: g.weight_vectors()                                                                                                        
Expected:                                                                                                                           
    [(3, 7, 1), (5, 1, 2), (7, 1, 4), (1, 1, 4), (1, 1, 1), (1, 4, 1), (1, 4, 10)]                                                  
Got:                                                                                                                                
    [(3, 7, 1), (5, 1, 2), (7, 1, 4), (5, 1, 4), (1, 1, 1), (1, 4, 8), (1, 4, 10)]
**********************************************************************

It is not obvious that the two answers are equivalent, I was going to submit it to the opinion of sage-devel on Monday.

On another note, do you know what this section of the Makefile is all about

ifeq ($(sagepath),)
SAGE_LINKOPTIONS = 
SAGE_INCLUDEOPTIONS =
else
SAGE_LINKOPTIONS = -L$(sagepath)/ -lpython2.6 -lcsage -lsingular
SAGE_INCLUDEOPTIONS = -I $(sagepath)/
SAGE_OBJECTS = sage.o sage_link.so

sage_link.so: sage_link.pyx setup.py
       python setup.py build_ext --inplace --pyrex-include-dirs=$(SAGE_ROOT)/devel/sage/
endif

It is not build by default and the cython source is missing (the resulting .cpp is present) but it looks like someone is looking at producing their own sage-extension.

comment:4 Changed 9 years ago by fbissey

I just had a quick look at your spkg. You didn't use "hg rm" to remove the files that are not needed anymore in patches. So hg status says this

francois@vrooom ~/Downloads/gfan-0.5 $ hg status
! patches/gcc45_linalg.cpp.patch
! patches/gcc45_matrix.h.patch
! patches/linalg.cpp
! patches/matrix.h
! patches/polynomial.cpp
! patches/polynomial.cpp.diff

comment:5 Changed 9 years ago by mhampton

OK, I removed the unneeded patches.

I don't think we want to change 'gfan' to 'gfan_bases' in the interface, that should be done in rings/polynomial/groebner_fan.py. I'll try to do that this week, along with other interface updates.

comment:6 Changed 9 years ago by fbissey

OK, looks better now. SPKG.txt could be more verbose about your changes (you dropped patches because they are not needed anymore and so on). However the spkg looks in good shape now. I'll give you a review (hopefully positive) once you post the patches.

comment:7 Changed 8 years ago by mjo

  • Status changed from new to needs_review

comment:8 Changed 8 years ago by fbissey

  • Status changed from needs_review to needs_work

We need some patch to use gfan 5.0 without at least generating noise. Putting in needs_work.

comment:9 Changed 8 years ago by leif

This probably needs to get rebased on #12760 (not yet merged, no positive review yet, but hopefully...)

(#12760 fixes C++11 issues with upstream code in 0.4plus, and also cleans up Sage's part.)

comment:10 Changed 8 years ago by fbissey

Is the patch safe to use with earlier version of gcc? c++, any standard, is a always a major source of headaches too me.

comment:11 Changed 7 years ago by mhampton

I hope to make some progress on this ticket for Sage-Combinat Days 40, July 9-13. So far, I have a working spkg on my mac, but I need to figure out if the gcc-4.5.0 patches need to be rebased for portability. I also want to add in some interface improvements (supporting symmetry for example).

In case anyone is interested in helping I will post my progress. Working spkg: http://sage.math.washington.edu/home/mhampton/gfan-0.5.p0.spkg

Current patch: http://sage.math.washington.edu/home/mhampton/trac_11395_update_gfan_to_0.5.patch

Last edited 7 years ago by mhampton (previous) (diff)

comment:12 Changed 7 years ago by fbissey

It is a pleasure to see some progress on this. I will have a look.

comment:13 Changed 7 years ago by fbissey

Patch works great but there are more instances of tests of gfan in the documentation:

sage -t -long -force_lib "devel/sage/doc/fr/tutorial/tour_advanced.rst"
**********************************************************************
File "/usr/share/sage/devel/sage/doc/fr/tutorial/tour_advanced.rst", line 68:
    sage: F.reduced_groebner_bases ()
Expected:
    [[-c^2 + b*d, -b*c + a*d, -b^2 + a*c],
     [c^2 - b*d, -b*c + a*d, -b^2 + a*c],
     [c^2 - b*d, b*c - a*d, -b^2 + a*c, -b^3 + a^2*d],
     [c^2 - b*d, b*c - a*d, b^3 - a^2*d, -b^2 + a*c],
     [c^2 - b*d, b*c - a*d, b^2 - a*c],
     [-c^2 + b*d, b^2 - a*c, -b*c + a*d],
     [-c^2 + b*d, b*c - a*d, b^2 - a*c, -c^3 + a*d^2],
     [c^3 - a*d^2, -c^2 + b*d, b*c - a*d, b^2 - a*c]]
Got:
    [[-c^2 + b*d, -b*c + a*d, -b^2 + a*c], [-c^2 + b*d, b^2 - a*c, -b*c + a*d], [-c^2 + b*d, b*c - a*d, b^2 - a*c, -c^3 + a*d^2], [c^3 - a*d^2, -c^2 + b*d, b*c - a*d, b^2 - a*c], [c^2 - b*d, -b*c + a*d, -b^2 + a*c], [c^2 - b*d, b*c - a*d, -b^2 + a*c, -b^3 + a^2*d], [c^2 - b*d, b*c - a*d, b^3 - a^2*d, -b^2 + a*c], [c^2 - b*d, b*c - a*d, b^2 - a*c]]

That's the French one but we have the same in English, Russian and German (de) version.

Changed 7 years ago by fbissey

Fix tutorial with gfan 0.5

comment:14 Changed 7 years ago by fbissey

Attaching a patch for the tutorial. Do you want me to clean up your spkg a little bit? We should only keep the patch for gcc 4.7 and really switch to patch. Of course SPKG.txt needs updating and cleaning after your work. There doesn't seem to be any tests so spkg-check. I can do some of that clean up if you want.

comment:15 Changed 7 years ago by mhampton

If you can do some of that soon (next two or three days), then go for it, that would be great. In the meantime I will focus on adding support for symmetry and maybe some other new things in gfan-0.5 like computing over ZZ.

comment:16 Changed 7 years ago by fbissey

OK I am doing this now. A bit more work than I had expected but doable.

comment:17 Changed 7 years ago by fbissey

The file app_minkowski.cpp had changes in it that are not in the patch file you shipped. I cannot see the equivalent modification in Gentoo for gcc-4.7.0 so I am skipping it.

comment:18 Changed 7 years ago by fbissey

  • Authors changed from Marshall Hampton to Marshall Hampton, François Bissey
  • Description modified (diff)
  • Reviewers set to François Bissey
  • Status changed from needs_work to needs_review

OK put a new spkg up. I have gone through the whole TODO list so the changes have been a bit invasive. I made a new Makefile that uses CXXFLAGS, LDFLAGS and CPPFLAGS - it could possibly use a little bit more love before sending upstream. Scrap that it would need a fair rewrite to get rid of a lot of stuff. Patch is now used throughout. Removal of the old gfan install is now done after the build and done correctly - which it wasn't before because of a misplaced double-quote.

I think that sums it up. Over to you Marshall to see if you like the whole thing.

comment:19 Changed 7 years ago by mhampton

  • Description modified (diff)

comment:20 Changed 7 years ago by mhampton

  • Description modified (diff)

comment:21 Changed 7 years ago by mhampton

I credited you (François Bissey) in the SPKG.txt, and I have uploaded a patch for the interface that I think greatly improves the functionality as well as supporting the new version. I am tempted to do more refactoring so I will not flag this for review quite yet.

comment:22 Changed 7 years ago by mhampton

I should mention that I am not really qualified to review the changes made to the spkg-install, but I will test this on a few more linux and OS X systems at least.

comment:23 Changed 7 years ago by fbissey

Actually I may make one more change. I defined PREFIX on the line calling make but the variable is never used. It would be if we were using the makefile to perform the installation but we don't. I tested the spkg on linux on power7. And as a final note I should add that gfan itself doesn't come with either tests or documentation. A curiosity is that gfan seem to be mostly c++ but link against libgmp rather than libgmpxx.

comment:24 Changed 7 years ago by fbissey

  • Description modified (diff)

OK I updated the spkg. In the end I kept the gcc-4.7 patch for app_minkowsky.cpp as before to minimize the burden of review.

As mentioned I removed the setting of PREFIX and made a comment about it.

comment:25 Changed 7 years ago by vbraun

I get a doctest error on sage-5.4.beta1 due to #12544

sage -t  devel/sage-main/sage/rings/polynomial/groebner_fan.py
**********************************************************************
File "/home/vbraun/opt/sage-5.4.beta1/devel/sage-main/sage/rings/polynomial/groebner_fan.py", line 504:
    sage: [q.facet_normals() for q in fan]
Expected:
    [(M(0, -1, 0), M(-1, 0, 0)), (M(0, 0, -1), M(-1, 0, 0)), (M(0, 0, 1), M(-1, 0, 0)), (M(0, 1, 0), M(-1, 0, 0)), (M(0, 0, -1), M(0, -1, 0)), (M(0, 0, 1), M(0, -1, 0)), (M(0, 1, 0), M(0, 0, -1)), (M(0, 1, 0), M(0, 0, 1)), (M(1, 0, 0), M(0, -1, 0)), (M(1, 0, 0), M(0, 0, -1)), (M(1, 0, 0), M(0, 0, 1)), (M(1, 0, 0), M(0, 1, 0))]
Got:
    [M( 0, -1, 0),
    M(-1,  0, 0)
    in 3-d lattice M, M( 0, 0, -1),
    M(-1, 0,  0)
    in 3-d lattice M, M( 0, 0, 1),
    M(-1, 0, 0)
    in 3-d lattice M, M( 0, 1, 0),
    M(-1, 0, 0)
    in 3-d lattice M, M(0,  0, -1),
    M(0, -1,  0)
    in 3-d lattice M, M(0,  0, 1),
    M(0, -1, 0)
    in 3-d lattice M, M(0, 1,  0),
    M(0, 0, -1)
    in 3-d lattice M, M(0, 1, 0),
    M(0, 0, 1)
    in 3-d lattice M, M(1,  0, 0),
    M(0, -1, 0)
    in 3-d lattice M, M(1, 0,  0),
    M(0, 0, -1)
    in 3-d lattice M, M(1, 0, 0),
    M(0, 0, 1)
    in 3-d lattice M, M(1, 0, 0),
    M(0, 1, 0)
    in 3-d lattice M]

comment:26 Changed 7 years ago by jdemeyer

  • Description modified (diff)

comment:27 Changed 7 years ago by jdemeyer

  • Status changed from needs_review to needs_work

needs_work because of Volker's failure.

Changed 7 years ago by vbraun

Initial patch

comment:28 Changed 7 years ago by vbraun

  • Description modified (diff)
  • Milestone changed from sage-5.4 to sage-5.5
  • Reviewers changed from François Bissey to François Bissey, Volker Braun
  • Status changed from needs_work to positive_review

Added a trivial fix, setting it back to positive review.

comment:29 Changed 7 years ago by jdemeyer

  • Status changed from positive_review to needs_work

The first patch needs a proper commit message.

Changed 7 years ago by vbraun

Added commit message

comment:30 Changed 7 years ago by vbraun

  • Description modified (diff)
  • Status changed from needs_work to positive_review

I've added the missing commit message.

comment:31 Changed 7 years ago by fbissey

Thanks I had completely forgotten about this.

comment:32 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.5.beta1
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.