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

MatrixSpace: Support constructing Hom(CombinatorialFreeModule) #37514

Merged
merged 12 commits into from
Apr 27, 2024

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Mar 1, 2024

The FreeModule constructor was recently generalized so that it can create CombinatorialFreeModule objects. In particular,

sage: ZZ^3
Ambient free module of rank 3 over the principal ideal domain Integer Ring
sage: ZZ^['a','b','c']
Free module generated by {'a', 'b', 'c'} over Integer Ring

Here as a follow-up, we extend MatrixSpace in a similar way: When lists or enumerated sets of row indices and column indices are given, it constructs CombinatorialFreeModules whose bases are indexed by these index sets, and then returns the homset. In particular,

sage: ZZ^(2, 3)
Full MatrixSpace of 2 by 3 dense matrices over Integer Ring
sage: ZZ^(['x','y'], ['a', 'b', 'c'])
Set of Morphisms from Free module generated by {'a', 'b', 'c'} over Integer Ring to Free module generated by {'x', 'y'} over Integer Ring in Category of finite dimensional modules with basis over Integer Ring

Follow-up PRs:

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 13, 2024

Ready to go?

@mkoeppe mkoeppe requested a review from gmou3 April 3, 2024 20:06
Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if you mix the dimension and keys? For example, row keys with number of columns. (Of course, there is more evil input of row keys and row dimension, which might be good to check against with doctests.)

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 4, 2024

It's checked for consistency, and errors are raised if there's a mismatch

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 4, 2024

Yes, I can add more doctests for that. If there are specific ones that you'd like to see, let me know

@tscrim
Copy link
Collaborator

tscrim commented Apr 4, 2024

Just doctests testing the compatibility, such as

MatrixSpace(QQ, ['a','b'], 4)
MatrixSpace(QQ, 4, ['a','b'])
MatrixSpace(QQ, ['a','b'], ['x','y'], nrows=2)  # I think should pass
MatrixSpace(QQ, ['a','b'], ['x','y'], nrows=4)
MatrixSpace(QQ, ['a','b'], ['x','y'], ncols=2)  # I think should pass
MatrixSpace(QQ, ['a','b'], ['x','y'], ncols=4)
MatrixSpace(QQ, ['a','b'], ['x','y'], nrows=2, ncols=2)  # I think should pass
MatrixSpace(QQ, ['a','b'], ['x','y'], nrows=2, ncols=4)
MatrixSpace(QQ, ['a','b'], ['x','y'], nrows=4, ncols=4)
MatrixSpace(QQ, 4, ['a','b'], nrows=4, ncols=2)  # I think should pass if the second above passes

Also, your INPUT: block and docstring is not correct. There is nrows_or_row_keys (which I think should be changed to row_keys) and similarly for columns.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 4, 2024

There is nrows_or_row_keys (which I think should be changed to row_keys)

It's intended as the name of a positional-only argument.

MatrixSpace(QQ, 4, ['a','b'], nrows=4, ncols=2) # I think should pass if the second above passes

No, I disagree, it should be rejected as a case of "duplicate argument" nrows.

@tscrim
Copy link
Collaborator

tscrim commented Apr 4, 2024

There is nrows_or_row_keys (which I think should be changed to row_keys)

It's intended as the name of a positional-only argument.

Ah, sorry I missed it in the long list of arguments. Now there are more compatibility tests to be done as you have to mix with the row_keys and nrows.

MatrixSpace(QQ, 4, ['a','b'], nrows=4, ncols=2) # I think should pass if the second above passes

No, I disagree, it should be rejected as a case of "duplicate argument" nrows.

It isn't a duplication per se. I could see it being used as a sanity check by a user. I would be a bit confused by this resulting in an error because of course it has 4 rows and 2 columns and the 4 is a shorthand for the index set [0, 1, 2, 3] in this setting.

Also, I believe this is true but I would like confirmation, the values that can be converted to int will always be less than sys.maxsize, correct?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 4, 2024

the 4 is a shorthand for the index set [0, 1, 2, 3] in this setting.

Well, there's a clear distinction between passing 4 and passing range(4).
One creates a FreeModule and the other creates a CombinatorialFreeModule.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 4, 2024

I have added the doctests (and input checking code), please take a look

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 4, 2024

the values that can be converted to int will always be less than sys.maxsize, correct?

No, int(x) converts any integer to an int. So we get, as expected,

sage: MatrixSpace(ZZ, sys.maxsize+1)
OverflowError: number of rows and columns may be at most 9223372036854775807

@tscrim
Copy link
Collaborator

tscrim commented Apr 6, 2024

the 4 is a shorthand for the index set [0, 1, 2, 3] in this setting.

Well, there's a clear distinction between passing 4 and passing range(4). One creates a FreeModule and the other creates a CombinatorialFreeModule.

If both of the arguments were integers, then I would agree. However, because one of the inputs is a list, passing 4 gets implicitly converted into a list before being passed along. Hence, it is not duplication.

@tscrim
Copy link
Collaborator

tscrim commented Apr 6, 2024

the values that can be converted to int will always be less than sys.maxsize, correct?

No, int(x) converts any integer to an int. So we get, as expected,

sage: MatrixSpace(ZZ, sys.maxsize+1)
OverflowError: number of rows and columns may be at most 9223372036854775807

That didn't exactly answer my question. I don't know what limits Python has imposed on the int right now, and while it might work for our systems, it should be universal.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 6, 2024

the 4 is a shorthand for the index set [0, 1, 2, 3] in this setting.

Well, there's a clear distinction between passing 4 and passing range(4). One creates a FreeModule and the other creates a CombinatorialFreeModule.

If both of the arguments were integers, then I would agree. However, because one of the inputs is a list, passing 4 gets implicitly converted into a list before being passed along.

No, there's no such mechanism. It constructs a morphism between a FreeModule and a CombinatorialFreeModule in this case.

      sage: MatrixSpace(QQ, ['a','b'], 4)
        Set of Morphisms (Linear Transformations)
         from Vector space of dimension 4 over Rational Field
           to Free module generated by {'a', 'b'} over Rational Field

Edit: I'll add that many of these homsets are not fully implemented, but that's outside of the scope of this PR. I have added some # known bug now to document this. For completing the implementation, see for example:

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 6, 2024

the values that can be converted to int will always be less than sys.maxsize, correct?

No, int(x) converts any integer to an int. So we get, as expected,

sage: MatrixSpace(ZZ, sys.maxsize+1)
OverflowError: number of rows and columns may be at most 9223372036854775807

That didn't exactly answer my question. I don't know what limits Python has imposed on the int right now, and while it might work for our systems, it should be universal.

I don't understand what you mean. What limits are you talking about?

@tscrim
Copy link
Collaborator

tscrim commented Apr 8, 2024

Okay, I've checked, in Python3, int is bounded by the size of the memory, which should always be larger than sys.maxsize (unless your machine is completely crazy).

@tscrim
Copy link
Collaborator

tscrim commented Apr 8, 2024

the 4 is a shorthand for the index set [0, 1, 2, 3] in this setting.

Well, there's a clear distinction between passing 4 and passing range(4). One creates a FreeModule and the other creates a CombinatorialFreeModule.

If both of the arguments were integers, then I would agree. However, because one of the inputs is a list, passing 4 gets implicitly converted into a list before being passed along.

No, there's no such mechanism. It constructs a morphism between a FreeModule and a CombinatorialFreeModule in this case.

      sage: MatrixSpace(QQ, ['a','b'], 4)
        Set of Morphisms (Linear Transformations)
         from Vector space of dimension 4 over Rational Field
           to Free module generated by {'a', 'b'} over Rational Field

Ah I see. This is acceptable then.

Although now I notice another issue. I don't quite like how, e.g., setting ncols overrides the CFM to a standard free module:

sage: MatrixSpace(QQ, ['a','b'], ['x','y'], ncols=2)
Set of Morphisms (Linear Transformations)
 from Vector space of dimension 2 over Rational Field
  to Free module generated by {'a', 'b'} over Rational Field

I am treating the ncols as verification here of the dimension.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 8, 2024

Although now I notice another issue. I don't quite like how, e.g., setting ncols overrides the CFM to a standard free module:

That's a bug.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 8, 2024

Fixed now

Copy link

github-actions bot commented Apr 8, 2024

Documentation preview for this PR (built with commit d26b9c8; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 16, 2024

Any more comments or can we merge it?

Copy link
Collaborator

@tscrim tscrim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is acceptable to me.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 19, 2024

Thanks!

vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 20, 2024
…ialFreeModule)`

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->
The `FreeModule` constructor was recently generalized so that it can
create `CombinatorialFreeModule` objects. In particular,
```
sage: ZZ^3
Ambient free module of rank 3 over the principal ideal domain Integer
Ring
sage: ZZ^['a','b','c']
Free module generated by {'a', 'b', 'c'} over Integer Ring
```

Here as a follow-up, we extend `MatrixSpace` in a similar way: When
lists or enumerated sets of row indices and column indices are given, it
constructs `CombinatorialFreeModule`s whose bases are indexed by these
index sets, and then returns the homset. In particular,
```
sage: ZZ^(2, 3)
Full MatrixSpace of 2 by 3 dense matrices over Integer Ring
sage: ZZ^(['x','y'], ['a', 'b', 'c'])
Set of Morphisms from Free module generated by {'a', 'b', 'c'} over
Integer Ring to Free module generated by {'x', 'y'} over Integer Ring in
Category of finite dimensional modules with basis over Integer Ring
```

Follow-up PRs:
- extend `matrix` likewise; guiding application: adjacency / incidence
"matrices" of graphs, where rows/columns are indexed by nodes and edges
- see also sagemath#35801

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#37514
Reported by: Matthias Köppe
Reviewer(s): Frédéric Chapoton, gmou3, Matthias Köppe, Travis Scrimshaw
@vbraun vbraun merged commit bf82b8e into sagemath:develop Apr 27, 2024
14 checks passed
@mkoeppe mkoeppe added this to the sage-10.4 milestone Apr 27, 2024
vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 28, 2024
…representation`: Support constructing `Hom(CombinatorialFreeModule)` elements

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

We use morphisms of `CombinatorialFreeModule`s (each of which has a
distinguished finite or enumerated basis indexed by arbitrary objects)
as matrices whose rows and columns are indexed by arbitrary objects
(`row_keys`, `column_keys`).

Example:
```
        sage: M = matrix([[1,2,3], [4,5,6]],
        ....:            column_keys=['a','b','c'], row_keys=['u','v']);
M
        Generic morphism:
          From: Free module generated by {'a', 'b', 'c'} over Integer
Ring
          To:   Free module generated by {'u', 'v'} over Integer Ring
```

Example application done here on the PR: The incidence matrix of a graph
or digraph. Returning it as a morphism instead of a matrix has the
benefit of keeping the vertices and edges with the result. This new
behavior is activated by special values for the existing parameters
`vertices` and `edges`.
```
            sage: D12 = posets.DivisorLattice(12).hasse_diagram()
            sage: phi_VE = D12.incidence_matrix(vertices=True,
edges=True); phi_VE
            Generic morphism:
              From: Free module generated by
                      {(1, 2), (1, 3), (2, 4), (2, 6), (3, 6), (4, 12),
(6, 12)}
                    over Integer Ring
              To:   Free module generated by {1, 2, 3, 4, 6, 12} over
Integer Ring
            sage: print(phi_VE._unicode_art_matrix())
                         (1, 2)  (1, 3)  (2, 4)  (2, 6)  (3, 6) (4, 12)
(6, 12)
                      1⎛     -1      -1       0       0       0       0
0⎞
                      2⎜      1       0      -1      -1       0       0
0⎟
                      3⎜      0       1       0       0      -1       0
0⎟
                      4⎜      0       0       1       0       0      -1
0⎟
                      6⎜      0       0       0       1       1       0
-1⎟
                     12⎝      0       0       0       0       0       1
1⎠
```



### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
- Depends on sagemath#37607
- Depends on sagemath#37514
- Depends on sagemath#37606
- Depends on sagemath#37646
    
URL: sagemath#37692
Reported by: Matthias Köppe
Reviewer(s): gmou3
vbraun pushed a commit to vbraun/sage that referenced this pull request May 2, 2024
…representation`: Support constructing `Hom(CombinatorialFreeModule)` elements

    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

We use morphisms of `CombinatorialFreeModule`s (each of which has a
distinguished finite or enumerated basis indexed by arbitrary objects)
as matrices whose rows and columns are indexed by arbitrary objects
(`row_keys`, `column_keys`).

Example:
```
        sage: M = matrix([[1,2,3], [4,5,6]],
        ....:            column_keys=['a','b','c'], row_keys=['u','v']);
M
        Generic morphism:
          From: Free module generated by {'a', 'b', 'c'} over Integer
Ring
          To:   Free module generated by {'u', 'v'} over Integer Ring
```

Example application done here on the PR: The incidence matrix of a graph
or digraph. Returning it as a morphism instead of a matrix has the
benefit of keeping the vertices and edges with the result. This new
behavior is activated by special values for the existing parameters
`vertices` and `edges`.
```
            sage: D12 = posets.DivisorLattice(12).hasse_diagram()
            sage: phi_VE = D12.incidence_matrix(vertices=True,
edges=True); phi_VE
            Generic morphism:
              From: Free module generated by
                      {(1, 2), (1, 3), (2, 4), (2, 6), (3, 6), (4, 12),
(6, 12)}
                    over Integer Ring
              To:   Free module generated by {1, 2, 3, 4, 6, 12} over
Integer Ring
            sage: print(phi_VE._unicode_art_matrix())
                         (1, 2)  (1, 3)  (2, 4)  (2, 6)  (3, 6) (4, 12)
(6, 12)
                      1⎛     -1      -1       0       0       0       0
0⎞
                      2⎜      1       0      -1      -1       0       0
0⎟
                      3⎜      0       1       0       0      -1       0
0⎟
                      4⎜      0       0       1       0       0      -1
0⎟
                      6⎜      0       0       0       1       1       0
-1⎟
                     12⎝      0       0       0       0       0       1
1⎠
```



### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [x] The title is concise and informative.
- [x] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation accordingly.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
- Depends on sagemath#37607
- Depends on sagemath#37514
- Depends on sagemath#37606
- Depends on sagemath#37646
    
URL: sagemath#37692
Reported by: Matthias Köppe
Reviewer(s): gmou3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants