-
-
Notifications
You must be signed in to change notification settings - Fork 491
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add class CircuitsMatroid #37148
Add class CircuitsMatroid #37148
Conversation
41a7d54
to
ed0f386
Compare
ed0f386
to
b17a2e3
Compare
1-line outputs and iterable to SetSystem
4ae07e2
to
bed97d8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to these comments, all def
and cpdef
methods need doctests.
src/sage/matroids/unpickling.pyx
Outdated
from sage.matroids.basis_matroid cimport BasisMatroid | ||
from sage.matroids.linear_matroid cimport LinearMatroid, RegularMatroid, BinaryMatroid, TernaryMatroid, QuaternaryMatroid | ||
from sage.matroids.circuit_closures_matroid cimport CircuitClosuresMatroid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not needlessly change the order of things, especially to something just as random.
Co-authored-by: Travis Scrimshaw <clfrngrown@aol.com>
Thanks for the detailed feedback. I was planning to further enrich the examples after the merging of this, and after editing some database matroids to be circuits matroids (the plan is to make the doctests much faster via a few strategic recasting of matroids into |
2179c8a
to
f38dce7
Compare
Change 'order' to 'ordering'; use 'ordering' for consistency with matroid.p*. Add nonspanning_circuits_iterator() and improve docstrings.
f38dce7
to
3767c65
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. It is looking good.
Here are some more comments. Some are necessary (again, all def
and cpdef
methods need doctests within their docstring), but others are some optimizations I think are fairly easy.
More examples and other mostly minor improvements Docstring improvements: 1-line references and outputs, 95 column limit in examples
48e45e5
to
d564669
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the changes.
I think the way you had the references before was better (and it is more in line with how we do other things in Sage generally).
A few more specific comments.
I think I found a solution for which we can both be happy (easily readable and avoids checks and indents). As for the references, by having them in one line we save a lot of whitespace. But I don't mind much either way; let me know if you want me to revert this. |
A few blanklines isn’t really a big deal, and the previous way was more readable with how it is formatted IMO. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. The only thing left is the order change in unpickling.pyx
. I think you wanted alphabetical, but it is not. I also am necessarily not in favor of these input sorting-type changes; yet, I won't reject it here (provided it is resorted by some reasonable way).
Documentation preview for this PR (built with commit e3b6119; changes) is ready! 🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then let is be so.
For the future, please avoid changing the import orders. Alphabetical is not always best and these unrelated changes can create unnecessary conflicts.
I strategically recast a few matroids in `database_matroids.py` to `CircuitsMatroid`s (given sagemath#37148). On my old laptop, this results in a `x5` speedup in the testing of the file, and in a `x2` speedup in the testing of the whole of the `matroids` module (saving `~1m`). URL: sagemath#37338 Reported by: gmou3 Reviewer(s): Travis Scrimshaw
This matroid subclass allows the definition and internal representation of a matroid based on its flats (closed sets). This representation can be advantageous for some algorithms. This completes the list of the most standard matroid definitions (bases, circuits, flats, rank function). This PR is similar to sagemath#37148. - [x] The title is concise, informative, and self-explanatory. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. - [x] I have updated the documentation accordingly. URL: sagemath#37340 Reported by: gmou3 Reviewer(s): gmou3, Matthias Köppe
Added a subclass
CircuitsMatroid
(of the mainMatroid
class). This enables the storage of a matroid based on its list of circuits, the verification using the circuit axioms, etc. The circuits description can be very succinct and it can facilitate efficient implementations of algorithms (one such example is the aforementioned verification).Up till now, when a matroid was defined via the keyword
circuits
-Matroid(circuits=[...])
- it was converted to aBasisMatroid
. This required the computation of the bases upon definition which could be very slow.For some relevant discussion, have a look at #36820 and #36956. This PR is my proposed resolution of #36820 (Note that this provides a workaround and not a direct implementation of the issue's request. In fact, I deem that a direct implementation is undesirable due to memory considerations; see #36956.)
📝 Checklist