Fix majorization again #41082
Fix majorization again #41082
Conversation
This cone induces the majorization preordering only vectors whose components are in nonincreasing order (i.e. its dual cone), not on the entire ambient space. This is obvious from the definition: the cone is defined by the same inequalities that appear in majorization, but in the latter context the entries are assumed to be nonincreasing. I corrected this in the test suite some time ago, but never updated the documentation.
Add +1 to a range(). The result is used to slice an array, where the comparison is already strict.
|
Documentation preview for this PR (built with commit f29eb1a; changes) is ready! 🎉 |
|
I suppose reviewing this requires some somewhat domain specific knowledge. Too bad the reviewer of the previous PR #37802 is away. (It's a mystery how [s: positive review] is removed while never added there.) Does any of the reviewer know the relevant math? Otherwise I guess I'm learning some new math. |
|
The bug in the test is related to the definition of https://en.wikipedia.org/wiki/Majorization. In R^n, there are (n-1) inequalities (and one equality) to check, pertaining to sums of the first i entries, for i=1,2,...,n-1. But the code was using To verify the documentation change, it isn't obvious from its generators, but the Schur cone is defined by the same inequalities (and one equality) that appear in the definition of majorization. (The cited references have this.) But with majorization, the components of the vector are rearranged in nonincreasing order first. So if you restrict yourself to the set of vectors in R^n with nonincreasing components, they're the same; if not, not. |
|
Thanks! If it's still buggy I can fix it again next year :) |
|
@cxzhong OK to set to positive review? |
I think it is OK |
|
Ok, thanks again |
sagemathgh-41082: Fix majorization again Fix sagemath#33906, which ultimately is just an off-by-one error. We also update the docs to mention that the induced preorder is only for a subset of the ambient space. (I should have done that the _last_ time I fixed this test.) URL: sagemath#41082 Reported by: Michael Orlitzky Reviewer(s): Chenxin Zhong
4 participants
Fix #33906, which ultimately is just an off-by-one error. We also update the docs to mention that the induced preorder is only for a subset of the ambient space. (I should have done that the last time I fixed this test.)