-
Notifications
You must be signed in to change notification settings - Fork 238
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 Vector == ZZ method #3606
base: development
Are you sure you want to change the base?
Add Vector == ZZ method #3606
Conversation
M2/Macaulay2/m2/matrix.m2
Outdated
@@ -119,6 +119,7 @@ Matrix == Matrix := (f,g) -> ( | |||
Matrix == Number := | |||
Matrix == RingElement := (m,f) -> m - f == 0 -- slow! | |||
Matrix == ZZ := (m,i) -> if i === 0 then rawIsZero m.RawMatrix else m - i == 0 | |||
Vector == ZZ := (v,i) -> matrix v == i |
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.
- Did you mean to also add
ZZ == Vector
? This might be a simple way to do both:
Vector == ZZ := (v,i) -> matrix v == i | |
Vector == ZZ := ZZ == Vector := (u, v) -> matrix u == matrix v |
- What you wrote also makes
vector {1} == 1
to be true, which is probably fine, but just making sure this is intended. - Is this automatically caught in a generic documentation node?
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.
- I didn't think to, but that's a good idea! I just pushed a new version adding this, and also supporting comparison using
==
withNumber
andRingElement
, matching the correspondingMatrix
methods. - Yes, that's intended.
- No, but I think that's a wider issue with all the
==
methods. None of them seem to be 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.
While you're at it, maybe also define Vector == Matrix
?
I don't particularly care about all of these nodes being documented, as long as it's obvious what they do. I do care about them not "missing documentation" though, so either marking as undocumented or globbing all missing nodes involving ==
seem like fine solutions to me. This would still allow documenting the ones that are noteworthy, like for modules/ideals/sheaves.
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.
Yeah, ==
is really bad for missing documentation nodes! https://macaulay2.com/doc/Macaulay2/share/doc/Macaulay2/Macaulay2Doc/html/__eq_eq.html
Some sort of automatic way for adding otherwise undocumented methods for common operators like +
, -
, and ==
to the main node for that operator sounds like great idea. But I think it's outside the scope of this PR.
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, then maybe don't touch the documentation at all.
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.
I figured I'd at least document the +
and -
methods that I added. The spacing between all the various keys for these nodes were super messy and inconsistent, so I decided I might as well clean it up a bit.
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.
If you want to clean up the messy documentation nodes then use globbing, otherwise why touch a hundred lines just to fix the indentation?
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.
Fair enough. I removed that commit and just added the new keys for now.
M2/Macaulay2/m2/matrix.m2
Outdated
Matrix == Number := | ||
Matrix == RingElement := | ||
Vector == Number := | ||
Vector == RingElement := (m,f) -> m - f == 0 -- slow! |
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.
Does this need to be written this way? This is slow due to #3035. For instance try v == 1
with v
the matrix defined in that issue.
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.
Also, Vector == Number
covers Vector == ZZ
, no?
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.
Do you have a suggestion for a better way to define Matrix == Number
and friends? That's probably outside the scope of this PR.
Regarding Vector == ZZ
, we currently need it to avoid a recursion error. Otherwise, something like v == 0
would call v - 0 == 0
.
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.
I guess I'm wondering what's wrong with m == matrix f
? The slight edge case is when the target of m has relations and you want to reduce f first, but maybe the user should explicitly write m - f * id_(target m)
in that case.
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.
That would break the current behavior since matrix f
is 1x1:
i1 : matrix {{3, 0}, {0, 3}} == 3
o1 = true
i2 : matrix {{3, 0}, {0, 3}} == matrix 3
o2 = false
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.
Sure, but this doesn't apply to vectors.
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.
Calling matrix
on the Number
/RingElement
won't work for the zero vector if the rank of the module is greater than one, since then we'll be comparing matrices with different sizes:
i1 : matrix vector {0, 0} == matrix 0
o1 = false
I did change it to just matrix v == f
, which works just fine:
i2 : matrix vector {0, 0} == 0
o2 = true
Then we don't have to worry about defining Vector == ZZ
.
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.
Instead of a long list of methods like this, which requires constant maintenance, I'd rather you glob them all like in 7bfcb62.
@@ -127,6 +134,8 @@ Matrix + RingElement := (f,r) -> if r == 0 then f else f + r*id_(target f) | |||
RingElement + Matrix := (r,f) -> if r == 0 then f else r*id_(target f) + f | |||
Number + Matrix := (i,f) -> if i === 0 then f else i*id_(target f) + f | |||
Matrix + Number := (f,i) -> if i === 0 then f else f + i*id_(target f) | |||
Vector + Number := Vector + RingElement := (v,r) -> vector(matrix v + r) | |||
Number + Vector := RingElement + Vector := (r,v) -> vector(r + matrix v) |
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.
i1 : Number + Vector := RingElement + Vector := (r,v) -> vector(r + matrix v);
i2 : 1 + (coker matrix {{ 10 }})_0
stdio:1:58:(3):[1]: error: expected source to be free with rank 1
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.
This is coming from vector(Matrix)
:
i1 : code (vector, Matrix)
o1 = -- code for method: vector(Matrix)
M2/Macaulay2/m2/modules.m2:66:17-69:5: --source code:
vector Matrix := f -> (
if not isFreeModule source f or numgens source f =!= 1 then error "expected source to be free with rank 1";
new target f from {f}
)
Should Vector
objects be elements of free modules? Or arbitrary modules?
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.
That's not the problem. Single generators of any module whatsoever are vectors. The implementation of Number + Vector
isn't correct because it uses the identity matrix on the target.
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.
Hmm, this seems to be a problem with matrix addition:
i1 : M = coker matrix 10;
i2 : A = map(M, ZZ^1, 1)
o2 = | 1 |
1
o2 : Matrix M <-- ZZ
i3 : 1 + A
o3 = | 2 |
o3 : Matrix M <-- M
i4 : A + 1
o4 = | 2 |
1
o4 : Matrix M <-- ZZ
Shouldn't they both have ZZ^1
as their source?
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.
I just pushed a new commit that should fix this
Vector == ZZ allows calling "zero" to determine whether a vector is the zero vector. Also add a unit test.
Previously, if the number/ring element appeared first, then the result was a matrix whose source *and* target was the target of the original matrix
Allows calling "zero" to test whether a vector is the zero vector.
Also add a unit test.
Before
After