-
-
Notifications
You must be signed in to change notification settings - Fork 510
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
Flag algebra implementation #36908
base: develop
Are you sure you want to change the base?
Flag algebra implementation #36908
Conversation
The docstring tests work well, but I couldn't build Error building the documentation. The above exception was the direct cause of the following exception: Traceback (most recent call last): |
This seems very interesting, but there are a number of things that need to be fixed first.
For getting the doc to build, I've found I needed to do I will provide more specific comments once the bigger things are more settled. |
Thank you for taking the time and looking at this PR! To reply to the points:
|
Sorry for taking so long to respond.
The class
You might want to reconsider which object should be the parent. There are also facade parents, which are things that act like a parent and "have" elements, but the resulting objects are associated with something else. For an example where this frequently occurs, see the posets code. However, this is all happening behind the scenes and shouldn't be a bottleneck; if it is, then the user should be more careful about where things live. Although other options for this will follow. If I was doing this myself, I would just let the coercion framework do its own thing. (As an example, look at the type of
Indeed, which is a strong indication that something should be done differently. As you have surmised, the coercion model is only about making sure you find a parent where the operation makes sense.
Just for clarity (sorry for some repetition), you should not be adding this. Coercion is very important core code that needs to run very fast and for very general objects.
Adding this would be a change to the coercion model, so I strongly think it should not be included. Again, sorry for some repetition in this. It seems like you either want to do what I said above or just let the coercion framework put things in a common parent where things make sense. In particular the
Not every method and function has a doctest from a quick look. This also includes
It gets you a little more speed and some better code localization. Also, from what you've said, why doesn't
I will check more carefully when I go through things line-by-line. Right now it is more about the bigger picture review.
There's a number of docstring issues from a quick look.
Some other comments:
|
Hi, if this PR needs help, either with linting (I guess I'm more familiar than bodnalev), or with reviewing, please ping me! It seems like a big PR but I think it will be useful for combinatorists, so it will be nice to have. |
3f205e4
to
a61e9c3
Compare
Thank you for the careful review of the pull request, and sorry for taking so long to get back. This was initially a quick rewrite of my custom flag algebra code to fit in the Sage framework. It turned out to be a larger project than I expected and I don't have loads of time to work on it next to my other commitments currently. @grhkm21 I appreciate any help regarding the documentation, especially with the formatting. I was mostly copy-pasting other documentation strings, seems like relatively unsuccessful. I've tried to move things around, and include more detail, but I imagine most of the formatting is still incorrect. Fixing the formatting errors is rather hard blindly, sorry I couldn't make much progress on it. A month ago I did a merge with the up-to-date branch. I suspected an issue with that, so I force-pushed a version where that merge is removed. Now, every change is after the 10.3.beta0 but it seems like that did not resolve the problem. Still no success with building the doc, and it seems the error isn't even related to the flag algebra code I have, as the original branch also fails to build. CombinatorialTheory is explained at the file level docstring. I've added an extra reference to it from the init. I understand the alternatives you've mentioned, but none really reflects the behaviour that I believe is intuitive. If you have a better alternative that satisfies the following conditions, then I can try to implement that:
I find this a little ridiculous, the code I included keeps the speed and generality. Unless there is an exception, it has the exact same speed. And the code just increases the generality. Now the elements can tell if there is a form that can behave as an operand. Stuff that lives in a space where, for example, multiplication is not defined, can coerce into something that can be multiplied when you try to multiply with it. For reference, see this accepted commit: |
This is a very weak argument. I am a combinatorialist, and the only place I have ever heard of a flag before this is as a sequence of vector spaces. On the contrary, because it is used in different settings means you should change the name to be able to differentiate it.
That's not quite correct. It is to convert everything to a common parent where the operation makes sense, and there are rules with what is allowed. If one parent does not coerce into the other, then they try to do a pushout construction. Although by your design, flags don't have operations that can be performed on them.
Yes, and there can be code that wants to handle those exceptions. I also do not see any real increase in generality either. You've added a check for an extra method that is used in precisely one place as a convenience (with no clear other uses at present).
I really don't understand this. The fact that you feel you need this is a code smell to me and your implementation or design needs to be changed. You wrote:
and
From this, it seems to me that the distinction you are making between
To begin, that's an apples to oranges comparison. Creating parents is called less frequently than coercion, but that is not used in initialization. Plus,
Again, that is not true, by your own admission. I don't know how much code catches the If you really want to change how coercion behaves, you should create a separate PR that clearly explains the rationale. However, I think there are much better ways to get the functionality you want (in order of correctness IMO):
|
The third David P. Robbins Prize was awarded to Alexander Razborov "for his paper, "On the minimal density of triangles in graphs" (Combinatorics, Probability and Computing 17 (2008), no. 4, 603-618), and for introducing a new powerful method, flag algebras, to solve problems in extremal combinatorics."
I think your analogy is not quite accurate, and therefore fails to illustrate the reason I changed coerce.pyx. A slightly better analogy would compare some set Now imagine you have no + operation on I know it depends on you trusting me, but I did try out alternatives, including pushout construction, overriding
It makes me wonder, why would such a code get accepted. Wouldn't it be easier to use knowledge about the specific situation and speed up the calculation even more, by skipping the checks in coercion (checking python types, checking numpy types, checking if the objects have a sage method ...)? And if we deal with hypothetical code scenarios that can potentially break due to a change in a core functionality, what if someone's code relied on _first_ngens not checking if the generators is a list, and instead wanted to catch the exception? What if someone wanted to use the R.<x,y,z> = QQ[] syntax millions of times? |
I didn't say it wasn't. Note that the flag manifold/variety (as a set of flags) has been around for longer in multiple fields.
This name collision must be solved before this gets merged into Sage. Perhaps
There is a natural map, but it is not canonical. Moreover, you have made one choice that is not natural nor canonical, choosing the field Now I think I understand how you are distinguishing between a flag and a flag algebra element. However, what you said about the uniqueness is misleading at best. You have chosen a base ring and a particular embedding to make it unique, but I could do that for any such injective function (say
This last statement here is not true as
Yes, but in that parent, there is no defined operation of, e.g., addition. So coercion should not try to do anything more and error out.
You would also need
I trust you that you tried. However, without seeing what you tried, I don't know if you made any mistakes in trying. (To this point, you would not want to use
I would be very surprised by such behavior as I would know that addition is not defined between the flags.
In addition to the ambiguity of the base ring, there are lots of subtleties involved here that you are not considering. What happens if you have two elements that would coerce into two objects that you would then apply the
In generic code, you might not know whether a particular coercion exist (which can be complicated because of checking sequences of coercions), but you then you might want to try something more extravagant (say, you know what a potential intermediate object is that you could "fill in" the change). Such a thing might be used in the coercion code to construct pushouts; I just don't know without digging back through that. Since this is such core speed-important code, I would much rather be safe. Independently of the speed considerations, changing the how the model works and its rules needs to be done with great care. There are currently strong rules/restrictions on what is allowed to be a coercion. See, e.g., https://doc.sagemath.org/html/en/reference/coercion/index.html This document (among others) would need to be amended with care with what would be allowed. This really needs to be done on a separate PR with a sage-devel discussion. Before that we can discuss some of the technicalities of the mathematical definition. Assuming we add this, how do you use just the elements (say, two flags) and the information contained within to define the addition?
I would be telling the author to bring this outside of the loop because there is no point in recreating the same object (well, the poly ring is pulled from the cache). Although if it was somehow necessary, then I would say use the explicit Let me again say I think this is a great piece of code that I really would like to get merged into Sage. Furthermore, I hope you will continue to contribute code to Sage after this as well. I know I am pushing back (very strongly) on something, but I hope that you don't think I am being hostile or unreasonable. |
@bodnalev, I am not sure whether you are aware of the fact that your patch currently modifies about 900 files. Maybe it would be good to rebase it, so one can see which files are "really" affected? |
Can handle any theory, any signature. A few interesting theories are included: -Graphs -Digraphs -Hypergrpahs -Graphs for Ramsey theory -Tournaments -Graphs with ordered edges -Graphs with ordered vertices It is designed to fit in the sage structure, FlagAlgebras can be constructed over any base ring containing QQ, and over any such combinatorial theory (graphs, hypergraphs etc). FlagAlgebraElements can be added and multiplied together interactively, with good performance and the powerful coercion model. Flag algebraic optimization problems can be solved with a small csdp wrapper csdpy (only working with linux at the moment). Includes docstring with examples, all finishing successfully. There is a small change to coerce.pyx to ask if the elements prefer a different form before applying an operation (so Flag is treated as a FlagAlgebraElement).
Added references to the bibliography and from other points of sage to flag algebras.
reply to commit: - I've added an extra link to Razborov's Flag Algebras paper, from the Flag file. I can attempt a greater explanation of what is happening there, but the paper's terminology is followed. The novelty is CombinatorialTheory, which comes with a detailed docstring. - The reason I changed coerce is the following: CombinatorialTheory is a parent, the elements are Flag-s. FlagAlgebra is a parent, the elements are FlagAlgebraElement-s. But a CombinatorialTheory contains all Flag-s, with every ftype, while a FlagAlgebra is only defined over a fixed ftype. But Flag-s, with a fixed ftype, form a basis for the FlagAlgebra with the same ftype. Therefore it makes sense to add and multiply the Flag-s together without the annoying step between, to change them to FlagAlgebraElement-s and then do the operation. As I was looking at the coercion model, I couldn't find a way to define coercion that depends on the elements. It wouldn't really make sense anyway. But when evaluating an expression like `Flag + Integer`, just from the parents, (CombinatorialTheory, Integers) it is impossible to figure out which FlagAlgebra they land in, since it depends on the Flag's ftype. So the extra code I added to the coercion is something really simple. It asks the operands (like the Flag in this example) if there is a form that actually behaves like an operand. And if they have a better form (Flag turns to FlagAlgebraElement) then use that better form in the operation. The code comes after all the other steps, so after the failure of all the other coercion attempts. If the `as_operand` is not implemented, then it acts just like the previous code, and there is only an overhead when an error occurs (which I assumed is fine since something went wrong anyway). I can add this to a separate PR. But the change only makes sense here, when I want to use this. I can add this explanation somewhere, if that helps. - All private functions should have tests now - I've experimented with that, but most of the heavy lifting is happening in Flag, I didn't see a reason to switch. - Thank you for catching those, I think I'm using the catalogs properly now, let me know if they are still bad. Also replaced `== None` with `is None` - I still can't build the reference manual. `make docbuild` gives this error: ``` The following package(s) may have failed to build (not necessarily during this run of 'make docbuild'): * package: sagemath_doc_html-none last build time: Dec 20 11:44 log file: /home/bodnalev/sage/logs/pkgs/sagemath_doc_html-none.log build directory: /home/bodnalev/sage/local/var/tmp/sage/build/sagemath_doc_html-none ``` - Made small fixes in the documentation
and clear code fix
output of the certificate is now in the original base verifier depends on only a small part of the code now.
With the X + epsilon I method
also bugs with typed optimization's rounding are fixed
and debug info for the exact rounding too
de506ed
to
45c06fb
Compare
|
||
|
||
A combinatorial theory is any theory with universal axioms only, | ||
(therefore the elements satisfy a heredetary property). This |
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.
(therefore the elements satisfy a heredetary property). This | |
(therefore the elements satisfy a hereditary property). This |
In addition we need two important functions: | ||
|
||
A generator function, showing the program how to generate elements of | ||
the theory for a fixed size. For example, to graphs with ordered vertices ( |
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.
the theory for a fixed size. For example, to graphs with ordered vertices ( | |
the theory for a fixed size. For example, graphs with a linearly ordered set of vertices ( |
the value is the actual list of edges. `'edges'` key is important to | ||
match the one defined in the signature. |
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.
the value is the actual list of edges. `'edges'` key is important to | |
match the one defined in the signature. | |
the value is the actual list of edges. It is important that the key -- `'edges'` -- | |
matches the one defined in the signature. |
-RamseyGraphTheory (see [LiPf2021]_ for explanation) | ||
|
||
The rest of this docstring will use `GraphTheory` since the number | ||
of structures is realtively small there. `Flag` docstring shows |
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.
of structures is realtively small there. `Flag` docstring shows | |
of structures is relatively small there. `Flag` docstring shows |
Dear @bodnalev, are you, in principle, interested in adding this to standard sage? |
Can handle any theory, any signature. A few interesting theories are included:
-Graphs
-Digraphs
-Hypergrpahs
-Graphs for Ramsey theory
-Tournaments
-Graphs with ordered edges
-Graphs with ordered vertices
It is designed to fit in the sage structure, FlagAlgebras can be constructed over any base ring containing QQ, and over any such combinatorial theory (graphs, hypergraphs etc). FlagAlgebraElements can be added and multiplied together interactively, with good performance and the powerful coercion model. Flag algebraic optimization problems can be solved with a small csdp wrapper csdpy (only working with linux at the moment).
Includes docstring with examples, all running successfully.
There is a small change to coerce.pyx to ask if the elements prefer a different form before applying an operation (so Flag is treated as a FlagAlgebraElement).
📝 Checklist
⌛ Dependencies