Skip to content
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

Cross-link matrix methods image and column_space #33548

Closed
slel opened this issue Mar 22, 2022 · 14 comments · Fixed by #34972
Closed

Cross-link matrix methods image and column_space #33548

slel opened this issue Mar 22, 2022 · 14 comments · Fixed by #34972

Comments

@slel
Copy link
Member

slel commented Mar 22, 2022

The image method of matrices mentions row_module (= row_space).

It should also mention column_space.

CC: @slel

Component: linear algebra

Branch/Commit: u/gh-Sandstorm831/33548 @ 6d17beb

Issue created by migration from https://trac.sagemath.org/ticket/33548

@slel slel added this to the sage-9.6 milestone Mar 22, 2022
@mkoeppe mkoeppe modified the milestones: sage-9.6, sage-9.7 Apr 11, 2022
@ackrue
Copy link

ackrue commented May 20, 2022

comment:2

I tried to take a look at this, but I couldn't find any mention of row_space in any of the matrix image functions, any of the documentation, or the wiki.

Can you clarify the specific method you're talking about?

@mkoeppe mkoeppe modified the milestones: sage-9.7, sage-9.8 Aug 31, 2022
@edgarcosta

This comment has been minimized.

@edgarcosta
Copy link
Member

comment:5

I have updated the ticket to reflect that row_module = row_space, and the former is the one used in the examples.

@Sandstorm831
Copy link
Contributor

Branch: u/gh-Sandstorm831/33548

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 19, 2023

Branch pushed to git repo; I updated commit sha1. New commits:

8278d5echanged row_space() to column_space()

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 19, 2023

Commit: 8278d5e

@Sandstorm831
Copy link
Contributor

comment:8

I have replaced row_space() in image method of a matrix to column_space() which I thought the to be wrong initially according to best of my knowledge. Please review and suggest edits if needed

@jhpalmieri
Copy link
Member

comment:10
sage -t --random-seed=142346370810930346368736796386774135144 sage/algebras/finite_dimensional_algebras/finite_dimensional_algebra.py  # 4 doctests failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/algebras/finite_dimensional_algebras/finite_dimensional_algebra_ideal.py  # 8 doctests failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/algebras/finite_dimensional_algebras/finite_dimensional_algebra_morphism.py  # 3 doctests failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/categories/finite_dimensional_algebras_with_basis.py  # 1 doctest failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/homology/chain_complex.py  # 1 doctest failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/homology/free_resolution.py  # 2 doctests failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/modular/abvar/abvar.py  # 46 doctests failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/modular/abvar/finite_subgroup.py  # 4 doctests failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/modular/abvar/morphism.py  # 6 doctests failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/modular/hecke/ambient_module.py  # 4 doctests failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/modular/hecke/element.py  # 3 doctests failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/modular/hecke/module.py  # 1 doctest failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/modular/hecke/submodule.py  # 6 doctests failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/modular/modform/ambient_g1.py  # 2 doctests failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/modular/modform/space.py  # 1 doctest failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/modular/modsym/ambient.py  # 1 doctest failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/modular/modsym/space.py  # 2 doctests failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/rings/function_field/differential.py  # 2 doctests failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/rings/function_field/element.pyx  # 2 doctests failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/rings/function_field/function_field.py  # 27 doctests failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/rings/function_field/ideal.py  # 4 doctests failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/rings/function_field/order.py  # 3 doctests failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/rings/function_field/place.py  # 3 doctests failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/schemes/curves/affine_curve.py  # 13 doctests failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/schemes/curves/projective_curve.py  # 18 doctests failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/schemes/generic/homset.py  # 1 doctest failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/schemes/riemann_surfaces/riemann_surface.py  # 17 doctests failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/schemes/toric/homset.py  # 15 doctests failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/schemes/toric/points.py  # 30 doctests failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/tests/book_stein_modform.py  # 1 doctest failed
sage -t --random-seed=142346370810930346368736796386774135144 sage/tests/books/computational-mathematics-with-sagemath/linalg_doctest.py  # 1 doctest failed

You can't change the behavior of a method like the image of a matrix so easily in a system like Sage. If you want to clarify the behavior, you could instead rewrite or add to the first sentence in the documentation: "Return the image of the homomorphism on rows defined by this matrix." I'm not sure what "on rows" means here, and that could be clarified.

You could in addition say "see also column_space" with some extra explanation. That's how I read the intent of the ticket from its description.

@jhpalmieri
Copy link
Member

comment:11

For example, you could say "Return the image of the homomorphism defined by right-multiplication by this matrix — that is, return the row space." Then perhaps insert a reference to row_space, and also a "see also column_space."

@Sandstorm831
Copy link
Contributor

comment:12

I am sorry sir, I misunderstood what the ticket want to convey. When I read about the image of matrices, I came to understand that image of a matrix is defined by its column space only, so I changed things accordingly. But Then I came to know the matrices in Sage act on the right, so as a result of which image of a matrix is its row_space instead of column space. I didn't realised that earlier. As here is a minor documentation addition, I will soon push another commits correcting my mistakes. sorry for any inconvenience.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 27, 2023

Changed commit from 8278d5e to 6d17beb

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 27, 2023

Branch pushed to git repo; I updated commit sha1. New commits:

5ce9dd7Revert "changed row_space() to column_space()"
6d17bebadded column_space in addition to row_space

@Sandstorm831
Copy link
Contributor

comment:14

Sir, please look at my following changes and please let me know if there is any thing else that needs to be done

@jhpalmieri
Copy link
Member

comment:15

First, you have to make this change:

diff --git a/src/sage/matrix/matrix2.pyx b/src/sage/matrix/matrix2.pyx
index 3830d0d3e9..46481df1bb 100644
--- a/src/sage/matrix/matrix2.pyx
+++ b/src/sage/matrix/matrix2.pyx
@@ -5213,7 +5213,7 @@ cdef class Matrix(Matrix1):
 
             sage: image(B) == B.row_module()
             True
-            sage: image(B) = B.transpose().column_module()
+            sage: image(B) == B.transpose().column_module()
             True
         """
         return self.row_module()

A first thing to do when trying out any change in Sage is to run doctests on the affected files; in this case, ./sage -t src/sage/matrix/matrix2.pyx would return a failure with your code, corrected with this fix.

Second, I've already suggested changes like these:

diff --git a/src/sage/matrix/matrix2.pyx b/src/sage/matrix/matrix2.pyx
index 3830d0d3e9..63f418bf41 100644
--- a/src/sage/matrix/matrix2.pyx
+++ b/src/sage/matrix/matrix2.pyx
@@ -5189,8 +5189,8 @@ cdef class Matrix(Matrix1):
 
     def image(self):
         """
-        Return the image of the homomorphism on rows defined by this
-        matrix.
+        Return the image of the homomorphism defined by right
+        multiplication by this matrix: that is, the row-space.
 
         EXAMPLES::
 

and maybe something like

diff --git a/src/sage/matrix/matrix2.pyx b/src/sage/matrix/matrix2.pyx
index 3830d0d3e9..af6be3c43e 100644
--- a/src/sage/matrix/matrix2.pyx
+++ b/src/sage/matrix/matrix2.pyx
@@ -5215,6 +5215,11 @@ cdef class Matrix(Matrix1):
             True
             sage: image(B) == B.transpose().column_module()
             True
+
+        .. SEEALSO::
+
+            :meth:`row_module`, :meth:`column_module`
+
         """
         return self.row_module()
 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants