-
-
Notifications
You must be signed in to change notification settings - Fork 453
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
Maximal angles between convex cones #37854
Maximal angles between convex cones #37854
Conversation
Documentation preview for this PR (built with commit 1b8f08f; changes) is ready! 🎉 |
6e0433d
to
66eba0e
Compare
c0a73cf
to
ab954c6
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.
Hopefully these changes are all speed improvements. I didn't check carefully that all statements are equivalent, but hopefully the intent is clear. Let me know if you have questions.
The phrase "this cone cannot be..." can be misinterpreted as making a value judgment about the given cone, rather than a blanket statement about the argument (self) to these two functions. This commit changes it to "cone should not be..." to indicate that the user should not pass in either a trivial cone or the ambient space.
…e(). The error message output when _random_admissible_cone() receives an invalid ambient dimension was a full sentence. This commit pares it down to just the important part; amely, that there are no nontrivial cones in the ambient dimension.
The line "G_index_sets = list(gevp_licis(G))" appears twice in EXAMPLES, but the variable "G_index_sets" is not used thereafter. It was most likely copy/pasted into the example by mistake. This commit removes it.
…30605. Thanks to some recent improvements in cone containment testing, we no longer have to reimplement it for vectors with irrational entries ourselves.
The sage library is now doctested with a random random seed (imagine), making most calls to set_random_seed() superfluous. We remove a few from the cone critical angle code here.
The solve_gevp_zero() and solve_gevp_nonzero() functions for cone critical angles are "internal" and can return generators. Likewise for the testing function _solve_gevp_naive(). This should be a tiny bit more efficient.
…metic. Passing exact=False to either the max_angle() or critical_angles() method of cones is a bit "dangerous" because we can miss eigenspaces of dimension >= 2 due to numerical issues. Add a warning to the docstring about it.
…s.py. Two tests in this module imported the gevp_licis() function but then did not use it. No longer.
We already have a matroid method that does more or less what the new gevp_licis() function does in cone_critical_angles.py. It's faster to use the matroid implementation and then tweak its output slightly, so let's do that instead.
For reasons I've long forgotten, the cone max/critical angles algorithms were using QQbar for "rationals with roots." Switching it to AA causes no problems now, and is clearly preferable (the only eigenvalues we compute are of positive-definite matrices).
26793ea
to
1e742e9
Compare
Using the identity M[I,J].transpose() == M.transpose()[J,I], we can avoid transposing M itself and instead transpose the smaller M[I,J] when M[I,J] is already known.
…ning The existing warning about this module being internal is easy to miss. This one should appear in bright red at the top of the module documentation.
…ngles This is an internal module, but it's fully documented and nice to have access to the implementation behind the max_angle() function. Despite being documented, the functions in cone_critical_angles.py are subject to change at any time. This itself is loudly documented.
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.
Okay, thank you. Then let it be so.
Thanks for the review! |
My colleagues are writing about this stuff again. Let's migrate sagemath#29169 from the old Trac branch. Relative to _develop_: * Add a new reference * Add a new module `src.sage.geometry.cone_critical_angles` for the implementation * Add a `max_angle()` method to the class in `src.sage.geometry.cone` for the user interface Relative to the old trac branch: * Update the algorithm to take https://www.heldermann.de/JCA/JCA31/JCA311/jca31015.htm into consideration * Add that paper as a reference * Drop the more-general critical angles computation. It isn't nearly as nice as the maximal angle method. TODO: * ~~Revert commit 95e5763 after checking the docs~~ URL: sagemath#37854 Reported by: Michael Orlitzky Reviewer(s): Kwankyu Lee, Matthias Köppe, Michael Orlitzky, Travis Scrimshaw
My colleagues are writing about this stuff again. Let's migrate sagemath#29169 from the old Trac branch. Relative to _develop_: * Add a new reference * Add a new module `src.sage.geometry.cone_critical_angles` for the implementation * Add a `max_angle()` method to the class in `src.sage.geometry.cone` for the user interface Relative to the old trac branch: * Update the algorithm to take https://www.heldermann.de/JCA/JCA31/JCA311/jca31015.htm into consideration * Add that paper as a reference * Drop the more-general critical angles computation. It isn't nearly as nice as the maximal angle method. TODO: * ~~Revert commit 95e5763 after checking the docs~~ URL: sagemath#37854 Reported by: Michael Orlitzky Reviewer(s): Kwankyu Lee, Matthias Köppe, Michael Orlitzky, Travis Scrimshaw
My colleagues are writing about this stuff again. Let's migrate sagemath#29169 from the old Trac branch. Relative to _develop_: * Add a new reference * Add a new module `src.sage.geometry.cone_critical_angles` for the implementation * Add a `max_angle()` method to the class in `src.sage.geometry.cone` for the user interface Relative to the old trac branch: * Update the algorithm to take https://www.heldermann.de/JCA/JCA31/JCA311/jca31015.htm into consideration * Add that paper as a reference * Drop the more-general critical angles computation. It isn't nearly as nice as the maximal angle method. TODO: * ~~Revert commit 95e5763 after checking the docs~~ URL: sagemath#37854 Reported by: Michael Orlitzky Reviewer(s): Kwankyu Lee, Matthias Köppe, Michael Orlitzky, Travis Scrimshaw
My colleagues are writing about this stuff again. Let's migrate #29169 from the old Trac branch.
Relative to develop:
src.sage.geometry.cone_critical_angles
for the implementationmax_angle()
method to the class insrc.sage.geometry.cone
for the user interfaceRelative to the old trac branch:
TODO:
Revert commit 95e5763 after checking the docs