sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.44k stars 480 forks source link

Matroids #7477

Closed 6bdad4c1-1e26-4f2f-a442-a01a2292c181 closed 11 years ago

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 14 years ago

Matroids in Sage could be interesting from the educational point of view, as there are not so many ways to play with matroids on a computer, but also from the algorithmic point of view, as the Graph Theory section could use some help from the Matroid Union and Matroid Intersection Theorems... ( see #7476 )

Macek is a GPL+C implementation of them http://www.fi.muni.cz/~hlineny/MACEK/ which I never tried but may be a good starting point !

Nathann

Apply:

Depends on #14669 Depends on #14668 Depends on #9880 Depends on #8386

CC: @kcrisman @sagetrac-yomcat

Component: combinatorics

Keywords: sd48

Author: Stefan van Zwam, Rudi Pendavingh

Reviewer: Volker Braun, Rob Beezer

Merged: sage-5.12.beta1

Issue created by migration from https://trac.sagemath.org/ticket/7477

wdjoyner commented 14 years ago
comment:1

I think adding macek to Sage will be a lot of work... .

Another option is to add my code http://boxen.math.washington.edu/home/wdj/sagefiles/matroid_class.sage I don't have time right now to add this to Sage properly, due to teaching obligations. However, if anyone is interested, I can at least act as one of the referees. If not, I will try to get to this next semester.

1d7ec08f-60ae-4512-91a6-8324c06eab9f commented 13 years ago
comment:2

Attachment: matroids-beezer-20110105.sage.gz

I attended a short course on matroids at the US national math meetings in January 2011, and spent the two days hacking up as much about matroids as I could. I knew David Joyner had done something similar, but I purposely did not look at his work first. So you will see some common ideas and some differences.

This is definitely not pretty, nor efficient. The goal was to implement as much functionality as quickly as possible, so there are obvious places where things should be done differently. But it is clear that much of the hard work can be shipped off to Sage routines for graph theory, linear algebra and combinatorics.

Implements vector matroid, cycle matroid, bicircular matroid, transversal matroid, uniform matroid, and duals of matroids.

90e8124e-743e-4424-88b0-22f5ae41372b commented 13 years ago
comment:3

There is serious interest from the matroid theory community in creating a Sage package. It would provide similar functionality to the graph theory package: lots of methods (extensions, representations, Tutte polynomials, connectivity tests, isomorphism and minor testing, ...) and a database with named matroids and small matroids.

Macek has several shortcomings that make it unsuitable; I will mention a few. It only works for representable matroids over the (small number of) partial fields implemented. It has an esoteric command language based on vertex-labelled trees. It has bugs. For instance, it cannot read its own output without modification, and is known not to detect minors when they are clearly present. Its author considers it "done", and will not support it.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 13 years ago
comment:4

(plus it would speedup the method Graph.edge_disjoint_spanning_tree which is deeeaaad slow right now)

1d7ec08f-60ae-4512-91a6-8324c06eab9f commented 13 years ago
comment:5

Replying to @sagetrac-Stefan:

Hi Stefan,

Thanks for the information on MACEK. I didn't know the details, but it looked to me like all support had ended, so it is good to have the confirmation.

What will it take to get the matroid community started with Sage?

Rob

wdjoyner commented 13 years ago
comment:6

Neither Rob's code nor mine is a patch. Any preference? I'm happy to convert my code into a patch and try to integrate Rob's new aspects in, or Rob can create a patch from his.

90e8124e-743e-4424-88b0-22f5ae41372b commented 13 years ago
comment:7

Replying to @rbeezer:

Replying to @sagetrac-Stefan:

What will it take to get the matroid community started with Sage?

Short answer (I sent you a longer by email): the matroid community has already started. We had a meeting in December, and will have a followup meeting in June. Several people have committed themselves to the effort.

By the way, I changed the "Component" field to combinatorics. I hope that's ok.

1d7ec08f-60ae-4512-91a6-8324c06eab9f commented 13 years ago
comment:9

Replying to @wdjoyner:

Neither Rob's code nor mine is a patch. Any preference? I'm happy to convert my code into a patch and try to integrate Rob's new aspects in, or Rob can create a patch from his.

Hi David,

Sorry for the delay in replying to this - recovering from Bug Days.

I think the computational matroid folks are quite serious about moving a lot of their work to Sage. Maybe it would be best if we let them decide what structure will work best for their purposes, rather than putting in something now that may not work well long-term?

You and I could probably best help them by advising and reviewing their contributions, I think.

But we shouldn't wait for them forever. ;-)

Rob

16bee037-a40f-49da-80f6-39ef26539405 commented 12 years ago
comment:10

I am brand new to Sage development, but I do a lot of work with matroids and would like to see them implemented in Sage.

I will be teaching a graduate course in algebraic combinatorics this fall. I am thinking of having my students create a Matroid sage class as a group project. E.g., they could implement the conversions between all the various ways of presenting a matroid; basic constructions like direct sum and dual; and the Tutte polynomial.

Thoughts?

Jeremy Martin (University of Kansas)

kcrisman commented 12 years ago
comment:11

Sounds like a great idea! You may want to try using the code attached here as a starting point. Also, the sage-combinat folks have some great infrastructure for things like categories, and you should ask them about that. But I think this is a very reasonable project. Especially if you could somehow implement graph.matroid() and/or vecspace.basis.matroid - in the sense that one could have a graph, and then get a matroid they could do stuff with from it.

90e8124e-743e-4424-88b0-22f5ae41372b commented 12 years ago
comment:12

Hi!

There's a significant effort going on behind the screens to get matroids into Sage. You can get to our work-in-progress code at

https://bitbucket.org/matroid/sage_matroids/

and we've got a mailing list at

https://groups.google.com/group/sage-matroid

Please join in the discussion. There is still plenty to be done.

A quick description of our current status: we've got an abstract Matroid class with a bunch of subclasses: BasisMatroid, LinearMatroid, RankMatroid, CircuitClosuresMatroid are the main ones. There is support for minors and abstract duality. We have a rather fast isomorphism test, and a host of methods for standard queries. There's also a library of common matroids, and a constructor that takes various inputs (including Sage graphs). So you can write

M = Matroid(G)

We think we need some minor coding and major documentation-string-writing before we want to submit it to Sage.

16bee037-a40f-49da-80f6-39ef26539405 commented 12 years ago
comment:13

Thanks! I will join that Google group and see what I can do to help.

Right now I am at a sage-combinat workshop at the IMA. There is significant interest among the algebraic combinatorialists here in implementing matroids for Sage; on the other hand, I'm not sure if the sage-combinat folks know about the sage-matroid project. I will make an announcement about it and invite people to get involved.

Jeremy

90e8124e-743e-4424-88b0-22f5ae41372b commented 11 years ago

Description changed:

--- 
+++ 
@@ -3,3 +3,8 @@
 Macek is a GPL+C implementation of them http://www.fi.muni.cz/~hlineny/MACEK/ which I never tried but may be a good starting point !

 Nathann
+
+Apply:
+
+* [attachment: trac_7477_setup_doc_load.patch](https://github.com/sagemath/sage-prod/files/10646823/trac_7477_setup_doc_load.patch.gz) -- changes to module_list.py, all.py, and reference manual
+* [attachment: trac_7477_code.patch](https://github.com/sagemath/sage-prod/files/10646822/trac_7477_code.patch.gz) -- code for the sage.matroids package
90e8124e-743e-4424-88b0-22f5ae41372b commented 11 years ago

Author: Stefan van Zwam, Rudi Pendavingh

90e8124e-743e-4424-88b0-22f5ae41372b commented 11 years ago
comment:15

Note: needs Sage 5.9.rc1 to build (because the package list is now generated automatically in setup.py, this used to be manual).

Note: the documentation builds fine from scratch, but I sometimes have trouble when adding these patches to an already built reference manual.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago
comment:16

This review will take a lifetime, but right now all I can say is that the code of circuits, cocircuits, noncospanning_cocircuits, and nonspanning_circuits look awfully similar.

And there are some parts of the code that are commented out, like that

+# def bitset_pickle_test(data):                                                                                                                                                               
+#     """                                                                                                                                                                                     
+#     Converts the list of integers ``data`` into a bitset, which gets pickled.                                                                                                               
+#     """                                                                                                                                                                                     
+#     cdef bitset_t bs                                                                                                                                                                        
+#     m = max(data)                                                                                                                                                                           
+#     bitset_init(bs, m+1)                                                                                                                                                                    
+#     bitset_clear(bs)                                                                                                                                                                        
+#     for i in data:                                                                                                                                                                          
+#         bitset_add(bs, i)                                                                                                                                                                   
+#     p = bitset_pickle(bs)                                                                                                                                                                   
+#     bitset_free(bs)                                                                                                                                                                         
+#     return p              

Or 4 functions in the matroid catalog.

Nathann

90e8124e-743e-4424-88b0-22f5ae41372b commented 11 years ago
comment:17

What do you mean by the comment that those functions look similar? They compute similar, but not identical, information from the matroid. And all four are useful to end users.

I guess we should have gotten rid of commented out parts (though Sage has plenty of that floating around in its source). I'll revise that soon.

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago
comment:18

Hellooooooo !

What do you mean by the comment that those functions look similar? They compute similar, but not identical, information from the matroid. And all four are useful to end users.

Nonono, of course you need them. I was just saying that perhaps there could have been an internal function computing all 4 things, exposed in 4 differents ways to the user. This way there is no copy/paste of code.

I guess we should have gotten rid of commented out parts (though Sage has plenty of that floating around in its source). I'll revise that soon.

Nonono I'm sorry I said that, I will give you lengthier reviews soon. Otherwise it will make you update the patch every day... :-)

Nathann

6bdad4c1-1e26-4f2f-a442-a01a2292c181 commented 11 years ago
comment:20

Hellooooooooooo again !

Well, I've been spending some time on that code, and I first have to say that it is awfully clean. I was a bit afraid that something developped outside of Sage would be more combinat-like, and that is not the case at all here.

The other thing is that, stupid as it may seem, I had not realized until a couple of minutes that I cannot realistically review 21 000 lines of code by myself. I mean, with a real job that I have to do, with 3 meals per day, everything. Looks complicated. This is one of the problems of developping everything for a while then trying to create a single patch out of it.

I will be glad to spend time on that, and also to work with the code later, but yep, I guess that I cannot review 21 000 lines of code :-)

I think that in this case, as the code looks preeeeetty clean and all, the best would really be to ask on sage-devel whether : 1) We take it in without a review (as if it were a spkg, actually) 2) Somebody (or many persons) are willing to review it together

Perhaps it would also help if you were to say in your message how you produced this code. If many persons wrote that code, if many tried it...

I think that I can't do more. Otherwise, I know that I will not set this to "positive review" before I am convinced that every single choice is a good one, or at least having asked about it. And after having thought for a while about each line, wondering if it is the best way to do it. I already spent weeks on easier reviews, and I know that I cannot do that one :-)

Nathann

90e8124e-743e-4424-88b0-22f5ae41372b commented 11 years ago
comment:21

I fully understand that you can't - I know I couldn't! I'll post on sage-devel to ask what's to be done.

--Stefan.

kcrisman commented 11 years ago
comment:22

Here are two alternate approaches to such a patch bomb.

I, too, was very impressed with the general layout of this project; a lot of thought has gone into this.

vbraun commented 11 years ago
comment:23

Looks pretty solid. I think you can review the mathematical correctness among yourself. You should have somebody who is more familiar with Sageisms to look at code style, then it should be ready to go. There are some small code style issues that would have been easier if you had started with a smaller chunk of code.

PEP8 whitespace http://www.python.org/dev/peps/pep-0008/#other-recommendations

i = i + 1   # yes
i=i+1       # no

Docstring markup http://www.sagemath.org/doc/developer/conventions.html#docstring-markup-with-rest-and-sphinx:

The SetSystem should probably be factored out and integrated into sage.sets.

Your private reimplementation of all matrix functionality has a lot of code-smell. If the only reason is that pivoting is too slow then you should look into fixing that instead of writing your own matrix implementation.

Whats this (left-over debugging?):

  #if d>0: 
  #    F=Fa+Fb 
  #    self._q_projection=self._q_projection.matrix_from_rows_and_columns(F,F) 
darijgr commented 11 years ago
comment:24

This looks wildly useful, not just for educational purposes. I always wanted to play around with the matroid Hopf algebra, for example...

The remarks below are mostly random and not always to the point. Most of the code is beyound my grasp due to the use of Cython and bitsets and partly due to advanced matroid theory. I'm just looking at random points which seem relevant and/or understandable to me.

What does this mean:

It is up to the child class to update its data structure make information  
relative to the new basis more accessible. 

Typo "an an":

if `I` is covered by a matching an an appropriate bipartite graph

That said, "covered by" might be useless, given that you're not saying "maximum matching".

In the next line, do you really mean "maximal matching", or rather "maximum matching"? (I don't know what you want.)

Typo "Compute a the rank". Also, look out for "the the" errors, you got 2 of them.

Any chances to get some comments on cdef __fundamental_cocircuit and cdef __fundamental_circuit? I can't say the code is self-explanatory... Is the "fundamental circuit" of a basis B and an element y the set of all elements of B that could be replaced by y, united with {y}? I see why this is a circuit (when y is not in B, that is) but I haven't seen this notation used anywhere...

Missing "of" in "of the flats the matroid" and in "of the coflats the matroid". Also, typos: "wheter", "distinguised", "commom".

Does the docstring of cpdef _augment(self, X, Y): want to say something like "nonempty (if possible) subset I" or "maximum subset I"? Just a subset is a bit boring...

Is matroid union implemented? I can't find it in the code...

The "if" in "with the property that if no modular triple of hyperplanes has exactly two members in the modular cut" looks out of place. Incidentally, a definition of the notion of "hyperplane" would be of use, too; I didn't know of that notion so far.

The max_weight_independent method raises an error if the ground set is empty and the weights keyword is set, something that might happen in practice in recursive definitions:

sage: M = matroids.Uniform(0,0)            
sage: wt = {}                             
sage: M.max_weight_independent(weights=wt)
---------------------------------------------------------------------------
IndexError                                Traceback (most recent call last)
<ipython-input-10-89b2ef4dae56> in <module>()
----> 1 M.max_weight_independent(weights=wt)

/home/darij/sage-5.9/local/lib/python2.7/site-packages/sage/matroids/matroid.so in sage.matroids.matroid.Matroid.max_weight_independent (sage/matroids/matroid.c:25219)()

/home/darij/sage-5.9/local/lib/python2.7/site-packages/sage/matroids/matroid.so in sage.matroids.matroid.Matroid.max_weight_independent (sage/matroids/matroid.c:24847)()

IndexError: list index out of range

This is due to the if wt[-1][1] < 0: line, of course. Same for max_weight_coindependent.

This just looks weird:

                if self.full_rank()==0: 
                return True             
            if self.full_rank()==0 or self.full_corank()==0: 
                return True  

One of the A's should probably be an Atranspose in:

                If the matroid is represented by `[I_1 A]`, then the dual is represented by `[-A I_2]` for appropriately sized 
            identity matrices `I_1, I_2`. 

(The minus sign, on the other hand, I don't see the reason for...)

Feature suggestion, if not already implemented: a method to test if a given weight function is generic, i. e., has exactly one maximizing basis. Of course, this is easy thanks to the exchange graph, as one only needs to find a maximizing basis and then check that all its exchange neighbours have strictly smaller weight. This function is useful to some Hopf-algebraic constructions.

90e8124e-743e-4424-88b0-22f5ae41372b commented 11 years ago
comment:25

Thanks for all your kind words, and for your suggestions! Michael Welsh and I worked on them, and the improved code is now on the ticket. Below I'll address your concerns one by one.

To ncohenn:

To vbraun:

To darij

darijgr commented 11 years ago
comment:26

Thanks for the answers! As for the maximizing bases, I just wanted a True or False; that alone should not be expensive.

I shouldn't be burdening you with additional work; I would normally be adding those methods myself if it wasn't for cython (the bitsets scared me as well, but that turned out to be unfounded). If you want people to add functionality, maybe you could document the internal functions better, such as __move_current_basis (not obvious from the title what it does; the same function with only 1 underscore is well-documented), and explain what the pattern behind 2 underscores vs. 1 underscores is (I guess 2-underscore functions take bitsets as input whereas 1-underscore ones use python types?). The doc of nxksrd is too brief; it suggests that the function returns the next k-subset, while apparently what it does is transforming the k-subset in place and returning something that is more like an error code. But take these suggestions with a grain of salt, as I'm not exactly an experienced programmer...

A few things you might still want to fix (sorry for not mentioning them earlier):

On an unrelated note: WTF our finite field matrices use floating-point algorithms???

90e8124e-743e-4424-88b0-22f5ae41372b commented 11 years ago
comment:27

I think you started studying the code in the wrong file. The main entry point is matroid.pyx. This is an abstract class of which all others derive. It implements all functionality in terms of just the rank function (ok... it'll convert to BasisMatroid for things like isomorphism testing). It should be fairly straightforward, and the code does not swerve far from pure Python.

BasisExchangeMatroid is a common framework for BasisMatroid and LinearMatroid. Internally the groundset is translated to a list of integers, which are used for bitset indexing. So we have

I think most people who will be adding code, will not move beyond the first underscore (things like union() belong in the generic Matroid class anyway). But certainly the cdef methods deserve a little bit of an explanation.

And yes:

sage: A = Matrix(GF(7), [[1,0,1,1],[0,1,1,2]])
sage: type(A)
sage.matrix.matrix_modn_dense_float.Matrix_modn_dense_float

In our matroid code we store the entries simply as a list of GF(q) elements, with some splicing commands for row operations. Results in a 10- to 20-time speedup in places. It's weird.

I hope I'll get around to doing the further edits soon!

vbraun commented 11 years ago
comment:28

Can you be more specific about which matrix functions are slow? And yes, it is intentional that matrices over certain finite fields are stored as floats --- with SSE you can do 4 float operations in one instruction.

90e8124e-743e-4424-88b0-22f5ae41372b commented 11 years ago
comment:29

vbraun: specifically, operations where the entries of the matrix (over GF(7), say) are changed frequently, if my memory serves me right. I'll come up with some speed tests in a week or two, but right now I'm overwhelmed with work and life.

Also: it looks like there are segmentation faults reported by the patchbot... I knew some things broke after Nathann's work on designs, will have to fix that.

90e8124e-743e-4424-88b0-22f5ae41372b commented 11 years ago
comment:30

I posted some info on slow matrix functions on https://groups.google.com/d/topic/sage-devel/5PdRIUic2Es/discussion

And the example is not contrived: we genuinely saw a 10- to 20-fold speedup in the LinearMatroid performance.

1d7ec08f-60ae-4512-91a6-8324c06eab9f commented 11 years ago
comment:31

I spent a good chunk of the afternoon looking over the documentation and wrestling with intersphinx.

Following are all pretty routine, though maybe the messages from exceptions needs discussion. Second post is about more mysterious stuff.

I'm doing

sage -docbuild reference html

and might turn to PDF output once this is settled. I'd be happy to ride herd on documentation as part of a group review. By and large, it looks excellent - I like many of the extras, like a discussion on subclassing and accurate synopses and lists at the top of modules. Nits:

(1) Set up code in the small patch all looks routine to my eye (except see next post). I have some experience with this, but will not say I am expert at it, so a second look is probably in order. (Interesting to see how little is actually required to add in a whole new field.) On the downside, having "matroid" at the top level is now going to make it harder to auto-complete the top-level "matrix" at the command line. ;-)

(2) In "Creating mew Matroid subclasses": "For incidental use, the RankMatroid subclass." needs another "use"?

(3) In documentation of matroid.is_isomorphism() the EXAMPLES all have an extra indent. Again with matroid.minor, matroid.equals. You might troll for more of these across all modules.

(4) I see "MatroidError" instances in the documentation, specifically here at matroid.max_independent. Is there a precedent for custom exceptions elsewhere in Sage? More commonly errors are TypeError or ValueError it seems to me. (Yes, PEP8 says differently.) And I find the "Problem with a matroid operation:" redundant. I believe it is a Python convention to have error messages begin with a lower case, to follow the colon (but cannot find a reference for this).

I'd be inclined to replace

MatroidError: Problem with a matroid operation: 'Input is not a subset of the groundset.'

with something like

ValueError: 'input set is not a subset of the groundset.'

Even better is to repeat the problem input in the error message. Here:

ValueError: 'input set ['x'] is not a subset of the groundset.'

There is usually enough context provided automatically, but echioing the bad input is often very helpful. Also consider making the text of error messages somewhat unique, so that searches will land a user at the right place in the reference manual.

(5) Apparently-minor documentation warnings follow. Should be trivial to fix.

[matroids ] /sage/sage-5.10.beta4/local/lib/python2.7/site-packages/sage/matroids/catalog.py:docstring of sage.matroids.catalog.NonVamos:3: WARNING: Inline interpreted text or phrase reference start-string without end-string.
[matroids ] /sage/sage-5.10.beta4/local/lib/python2.7/site-packages/sage/matroids/catalog.py:docstring of sage.matroids.catalog.P8pp:1: WARNING: Inline interpreted text or phrase reference start-string without end-string.
[matroids ] /sage/sage-5.10.beta4/local/lib/python2.7/site-packages/sage/matroids/catalog.py:docstring of sage.matroids.catalog.P8pp:1: WARNING: Inline interpreted text or phrase reference start-string without end-string.
[matroids ] <autodoc>:0: WARNING: Bullet list ends without a blank line; unexpected unindent.
1d7ec08f-60ae-4512-91a6-8324c06eab9f commented 11 years ago
comment:32

Some errors with building the HTML documentation follow. I think this is from using intersphinx, but am not certain. Perhaps also related to the conf.py files. I'll ping John Palmieri off-list to see if he wants to take a look. On 5.10.beta2, but very similar behaviour on 5.10.beta4.

Two files in devel/sage/doc/en/reference: conf.py and conf_sub.py. The first has a long list of top-level sections of the documentation. But to my eye it seems we use the second and not the first (and setup patch here also seems to reference the second).

I added 'matroids' to the first and tried rebuilding (two passes). Then tried rebuilding with

sage -docbuild reference html -S -aE

which my notes say will for a rebuild from scratch.

Here are the symptoms I'm trying to fix. Many many of these, and indeed the objects.inv is not created for the matroids section.

[homology ] WARNING: intersphinx inventory '/sage/sage-5.10.beta2/devel/sage/doc/output/html/en/reference/matroids/objects.inv' not fetchable due to <type 'exceptions.IOError'>: [Errno 2] No such file or directory: '/sage/sage-5.10.beta2/devel/sage/doc/output/html/en/reference/matroids/objects.inv'

I get about seven of these. Perhaps it is due to cdef'ed class definitions. That is a guess. However, limited Googling suggests this can be fixed with a proper conf.py, so maybe another symptom of the same problem. For each file/module, it seem severe enough to keep the whole module's worth of documentation from rendering at all - there is just a title, and no real content.

[reference] /sage/sage-5.10.beta2/devel/sage/doc/en/reference/matroids/sage/matroids/basis_exchange_matroid.rst:11: WARNING: autodoc can't import/find module 'sage.matroids.basis_exchange_matroid', it reported error: "'module' object has no attribute 'BasisExchangeMatroid'", please check your spelling and sys.path
90e8124e-743e-4424-88b0-22f5ae41372b commented 11 years ago
comment:33

Hi,

I've had trouble building the documentation. I guess adding an extra folder is not easily accounted for. My recipe was

I'll get to the other issues mentioned soon.

tscrim commented 11 years ago
comment:34

Hey Rob,

It's saying it can't find that file /sage/sage-5.10.beta2/devel/sage/doc/output/html/en/reference/matroids/objects.inv. It probably doesn't exist. When doing top-level doc stuff, I always run

sage -docbuild all html

I'd try that first.

Also I'm somewhat scared of deleting the entire output directory now. I once did that and consistently got the error your is getting, even doing docbuild all (I ended up reinstalling sage, but that's more because I wanted to upgrade anyways.) Perhaps make doc does a few extra things then docbuild all does...?

Stefan,

The bullet points need to be formatted like this:

Some text saying a list:

- the first level, note the blank line
- but this needs a sublist:

  - Note the indentation and the blank line inbetween.

- A line which needs a break
  will go like this.

I believe that should take care of the sphinx errors.

Best,

Travis

1d7ec08f-60ae-4512-91a6-8324c06eab9f commented 11 years ago
comment:35

Dear Travis,

Thanks for the suggestions.

Replying to @tscrim:

sage -docbuild all html

I'd try that first.

That's looking much better! And without editing a conf.py I'll take it up more seriously in the morning.

Also I'm somewhat scared of deleting the entire output directory now. I once did that and consistently got the error your is getting, even doing docbuild all (I ended up reinstalling sage, but that's more because I wanted to upgrade anyways.) Perhaps make doc does a few extra things then docbuild all does...?

Well, I just nuked all of doc/output to make it rebuild and it seems to have run fine. Again, I'll double-check in the AM. I wonder what is different, and what needs to change to get -docbuild to just do one part of the docs?

Thanks, Rob

vbraun commented 11 years ago
comment:36

Deleting doc/output is supposed to work, I do it all the time and its works fine. There might have been bugs in specific older Sage versions, though ;-)

vbraun commented 11 years ago
comment:37

Reading more of the code, is there a reason for why the bitset enhancements don't go into sage.misc.bitset? They seem to be useful enough, so why hide it in the matroid directory?

90e8124e-743e-4424-88b0-22f5ae41372b commented 11 years ago
comment:38

I just uploaded a new version of the patch. I tested it against 5.10.beta4, I hope it didn't break on 5.10.beta5...

Items fixed:

Responding to darij:

Responding to rbeezer:

Responding to vbraun (some comments are replies to issues from sage-devel):

The LinearMatroid class only uses the trivial nonstandard is_nonzero method now. The subclasses BinaryMatroid, TernaryMatroid, QuaternaryMatroid, RegularMatroid use a few more methods that have no Sage Matrix equivalent, but all are flagged in the code, and it should be possible to replace them. One last remark: the lean_matrix.pyx code is ONLY used as internal data structure, we don't expose any of it to the user (unless they try really hard). Everything the user sees is a normal Sage matrix.

vbraun commented 11 years ago
comment:39

I get some docbuild errors, e.g.

[reference] /home/vbraun/opt/sage-5.10.beta5/devel/sage/doc/en/reference/matroids/sage/matroids/basis_exchange_matroid.rst:11: WARNING: autodoc can't import/find module 'sage.matroids.basis_exchange_matroid', it reported error: "'module' object has no attribute 'BasisExchangeMatroid'", please check your spelling and sys.path
[matroids ] /home/vbraun/opt/sage-5.10.beta5/devel/sage/doc/en/reference/matroids/index.rst:7: WARNING: toctree contains reference to nonexisting document 'matroids/sage/matroids/constructor'
vbraun commented 11 years ago
comment:40

PS: the fact that you are more likely to get conflicts when you also improve other parts of Sage is not an excuse for not doing it ;-) You should have separated out the bitset improvements into a different ticket, this would have made this ticket less of a patch bomb, helped with reviewing, and made conflicts (if any) more manageable.

90e8124e-743e-4424-88b0-22f5ae41372b commented 11 years ago
comment:41

Yes, you're right, I didn't look closely enough at the docbuild process. It is related to lazy_import: if I change all.py to do a regular import, then the documentation builds just fine... I guess I'll have to ask sage-devel for advice on that.

And opening a separate ticket: that's definitely a good solution, why didn't I think of that?

1d7ec08f-60ae-4512-91a6-8324c06eab9f commented 11 years ago
comment:42

Replying to @vbraun:

I get some docbuild errors, e.g.

Me, too. I got a bit of advice from John Palmieri off-list, but that did not do the trick. FWIW, no matter how hard I try to rebuild from scratch, these persist. I was about to poll sage-devel, but I see that has started.

Once this gets sorted, I should be able to finish a review of the documentation part of this.

Rob

jhpalmieri commented 11 years ago
comment:43

This change ought to fix the docbuilding problems:

diff --git a/doc/en/reference/conf.py b/doc/en/reference/conf.py
--- a/doc/en/reference/conf.py
+++ b/doc/en/reference/conf.py
@@ -94,6 +94,7 @@
     'libs',
     'logic',
     'matrices',
+    'matroids',
     'misc',
     'modabvar',
     'modfrm',

However, I think a better solution is to auto-generated this list. Volker, you know the docbuild system. Can you look at my patch?

jhpalmieri commented 11 years ago

Description changed:

--- 
+++ 
@@ -8,3 +8,4 @@

 * [attachment: trac_7477_setup_doc_load.patch](https://github.com/sagemath/sage-prod/files/10646823/trac_7477_setup_doc_load.patch.gz) -- changes to module_list.py, all.py, and reference manual
 * [attachment: trac_7477_code.patch](https://github.com/sagemath/sage-prod/files/10646822/trac_7477_code.patch.gz) -- code for the sage.matroids package
+* [attachment: trac_7477-docbuild.patch](https://github.com/sagemath/sage-prod/files/10646820/trac_7477-docbuild.patch.gz) -- autogenerate the list of subdirectories of `doc/en/reference` which contain components of the reference manual
jhpalmieri commented 11 years ago
comment:44

Attachment: trac_7477-docbuild.patch.gz

(If you want, we can move my patch to a different ticket and have it be a prerequisite for this one.)

1d7ec08f-60ae-4512-91a6-8324c06eab9f commented 11 years ago
comment:45

Replying to @jhpalmieri:

This change ought to fix the docbuilding problems:

Yes, it does, thanks. I had tried that, but was just trying to build the reference only and there was no inventory file. So when you apply the patches here, try

sage -docbuild all html

first to get a proper collection of HTML documentation. Maybe John's change should just go in Stefan's setup patch - seems like that is where it belongs. Autogeneration could be something new?

And I'm wondering if inventories should get rebuilt on smaller units of the documentation. I'll ask that on sage-devel so as to not clutter this any further.

jhpalmieri commented 11 years ago
comment:46

You should just be able to do sage --docbuild reference/matroids inventory and then sage --docbuild reference/matroids html. Building the documentation this way is a lot like running LaTeX: you need to run it several times to resolve references, links, etc. Running sage --docbuild all html (and the same with pdf in place of html) automates the process, building the docs twice. Using any other options just builds once. (This was a design decision discussed at #6495.)

1d7ec08f-60ae-4512-91a6-8324c06eab9f commented 11 years ago
comment:47

Thanks, John. Just obsoleted much of my post to sage-devel. ;-) I'd read some of #6495 but not carefully enough to follow all the nuances.

I've been away too long - need to get caught up. Sorry for the noise.

90e8124e-743e-4424-88b0-22f5ae41372b commented 11 years ago
comment:48

I can confirm that the docbuild patch fixes our problems. I also found that the PDF didn't build. The current update fixes that.

jdemeyer commented 11 years ago
comment:49

Replying to @vbraun:

PS: the fact that you are more likely to get conflicts when you also improve other parts of Sage is not an excuse for not doing it ;-) You should have separated out the bitset improvements into a different ticket, this would have made this ticket less of a patch bomb, helped with reviewing, and made conflicts (if any) more manageable.

Completely agreed.

vbraun commented 11 years ago
comment:50

John's docbuilding patch looks good to me.

Rob: are you still proof reading the documentation?