Improve documentation for the "Matrix groups" section #4556

ThomasBreuer · 2025-02-06T21:16:51Z

add introductory text and examples for matrix groups (analogous to the recent changes in the documentation for f.p. groups)
improve the documentation for sesquilinear forms
change/fix docstrings, add examples to docstrings
export invariant_bilinear_form, invariant_quadratic_form, invariant_sesquilinear_form (alternatively, we could remove their documentation from the manual)

- add introductory text and examples for matrix groups (analogous to the recent changes in the documentation for f.p. groups) - improve the documentation for sesquilinear forms - change/fix docstrings, add examples to docstrings - export `invariant_bilinear_form`, `invariant_quadratic_form`, `invariant_sesquilinear_form` (alternatively, we could remove their documentation from the manual)

fingolfin · 2025-02-06T23:24:16Z

src/Groups/matrices/form_group.jl

@@ -184,7 +184,7 @@ An exception is thrown if `base_ring(G)` is not a finite field with even degree
 over its prime subfield.

 !!! warning "Note:"
-    At the moment, elements of the generating set are returned of type `mat_elem_type(G)`.
+    At the moment, elements of the generating set are returned of type `mat_elem_type(typeof(G))`.


For other similar *_type function we actually make sure that one can also pass in instances, for convenience. Maybe a good idea to do that here, too? E.g. we do this in AA:

elem_type(x) = elem_type(typeof(x)) elem_type(T::DataType) = throw(MethodError(elem_type, (T,)))

I found mat_elem_type a bit irritating: It is not documented at all, but it appears essentially in these warnings in the documentation.

Now I think that the function is useful. Thus I will document mat_elem_type and ring_elem_type, and then it makes sense to admit also the group as an argument instead of its type.

codecov · 2025-02-06T23:37:52Z

Codecov Report

Attention: Patch coverage is 70.58824% with 5 lines in your changes missing coverage. Please review.

Project coverage is 85.40%. Comparing base (ca6338c) to head (abfcfa7).
Report is 99 commits behind head on master.

Files with missing lines	Patch %	Lines
src/Groups/matrices/MatGrp.jl	68.75%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4556      +/-   ##
==========================================
+ Coverage   84.43%   85.40%   +0.96%     
==========================================
  Files         673      677       +4     
  Lines       89624   101380   +11756     
==========================================
+ Hits        75676    86582   +10906     
- Misses      13948    14798     +850

Files with missing lines	Coverage Δ
src/Groups/matrices/form_group.jl	`97.56% <ø> (ø)`
src/Groups/matrices/forms.jl	`93.25% <100.00%> (ø)`
src/Groups/matrices/linear_centralizer.jl	`100.00% <ø> (ø)`
src/Groups/matrices/linear_isconjugate.jl	`100.00% <ø> (ø)`
src/Groups/matrices/matrix_manipulation.jl	`98.59% <ø> (ø)`
src/Groups/matrices/transform_form.jl	`98.51% <ø> (ø)`
src/Groups/matrices/MatGrp.jl	`94.44% <68.75%> (+0.08%)`	⬆️

... and 152 files with indirect coverage changes

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- document `ring_elem_type`, `mat_elem_type`, admit a matrix group as argument - add examples for `corresponding_bilinear_form`, `corresponding_quadratic_form` - changed `show` for forms to avoid trailing whitespace

The matrices returned by `corresponding_bilinear_form` and `corresponding_quadratic_form` are determined at most up to scalars, thus `jldoctest` cannot be used.

ThomasBreuer · 2025-02-07T13:45:56Z

Some tests fail: Aqua's "persistent tasks" complains.
Can the changes from this pull request cause this problem?

fingolfin · 2025-02-20T00:56:43Z

I don't think the Aqua issue are from this PR.

This PR is still marked as draft, please let me know when you think it is ready for review.

fingolfin · 2025-02-28T15:05:27Z

src/Groups/matrices/MatGrp.jl

+```jldoctest
+julia> g = GL(2, 3);
+
+julia> ring_elem_type(typeof(g)) == elem_type(typeof(base_ring(g)))


So this is also the same as this then?

Suggested change

julia> ring_elem_type(typeof(g)) == elem_type(typeof(base_ring(g)))

julia> ring_elem_type(typeof(g)) == elem_type(base_ring_type(g))

Makes me wonder if we really need ring_elem_type. Ah well, I guess we have it for now...

The change from typeof(base_ring(X)) to base_ring_type(X) would make sense across Oscar (perhaps for another pull request).

fingolfin · 2025-03-31T09:37:03Z

src/Groups/matrices/forms.jl

+```@repl
+Q = quadratic_form(invariant_quadratic_form(GO(3,3)))
+corresponding_bilinear_form(Q)
+```


Any reason not to use a doctest here?

Suggested change

```@repl

Q = quadratic_form(invariant_quadratic_form(GO(3,3)))

corresponding_bilinear_form(Q)

```

```jldoctest

julia> Q = quadratic_form(invariant_quadratic_form(GO(3,3)))

quadratic form with Gram matrix

[0 1 0]

[0 0 0]

[0 0 1]

julia> corresponding_bilinear_form(Q)

symmetric form with Gram matrix

[0 1 0]

[1 0 0]

[0 0 2]

```

fingolfin · 2025-03-31T09:37:14Z

src/Groups/matrices/forms.jl

 It is defined only in odd characteristic.
+
+# Examples
+```@repl


fingolfin

Overall this looks good to me (I would consider changing the examples using @repl to use jldoctest instead but that's a minor point).

If there is something blocking this, it would be good to know what it is, @ThomasBreuer. Otherwise I hope we can just merge this soonish?

ThomasBreuer added documentation Improvements or additions to documentation topic: groups release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Feb 6, 2025

fix a doctest

f1cabb1

fingolfin reviewed Feb 6, 2025

View reviewed changes

ThomasBreuer added 2 commits February 7, 2025 11:04

address comments

b4b4274

- document `ring_elem_type`, `mat_elem_type`, admit a matrix group as argument - add examples for `corresponding_bilinear_form`, `corresponding_quadratic_form` - changed `show` for forms to avoid trailing whitespace

change docstrings for examples with random effects

abfcfa7

The matrices returned by `corresponding_bilinear_form` and `corresponding_quadratic_form` are determined at most up to scalars, thus `jldoctest` cannot be used.

ThomasBreuer marked this pull request as draft February 10, 2025 09:41

fingolfin reviewed Feb 28, 2025

View reviewed changes

fingolfin closed this Mar 11, 2025

fingolfin reopened this Mar 11, 2025

fingolfin reviewed Mar 31, 2025

View reviewed changes

src/Groups/matrices/forms.jl

It is defined only in odd characteristic.

# Examples

```@repl

Copy link

Member

fingolfin Mar 31, 2025

Choose a reason for hiding this comment

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

ditto

fingolfin approved these changes Mar 31, 2025

View reviewed changes

Merge branch 'master' into TB_doc_matgroups_intro

b1aa1eb

ThomasBreuer marked this pull request as ready for review April 1, 2025 07:43

fingolfin merged commit 4f654db into oscar-system:master Apr 1, 2025
30 of 32 checks passed

ThomasBreuer deleted the TB_doc_matgroups_intro branch April 1, 2025 11:59

ThomasBreuer mentioned this pull request Apr 4, 2025

Turn two @repl blocks into jldoctest #4776

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve documentation for the "Matrix groups" section #4556

Improve documentation for the "Matrix groups" section #4556

ThomasBreuer commented Feb 6, 2025

fingolfin Feb 6, 2025

ThomasBreuer Feb 7, 2025

codecov bot commented Feb 6, 2025 •

edited

Loading

ThomasBreuer commented Feb 7, 2025

fingolfin commented Feb 20, 2025

fingolfin Feb 28, 2025

ThomasBreuer Apr 1, 2025

fingolfin Mar 31, 2025

fingolfin Mar 31, 2025

fingolfin left a comment

	julia> ring_elem_type(typeof(g)) == elem_type(typeof(base_ring(g)))
	julia> ring_elem_type(typeof(g)) == elem_type(base_ring_type(g))

Improve documentation for the "Matrix groups" section #4556

Improve documentation for the "Matrix groups" section #4556

Conversation

ThomasBreuer commented Feb 6, 2025

fingolfin Feb 6, 2025

Choose a reason for hiding this comment

ThomasBreuer Feb 7, 2025

Choose a reason for hiding this comment

codecov bot commented Feb 6, 2025 • edited Loading

Codecov Report

ThomasBreuer commented Feb 7, 2025

fingolfin commented Feb 20, 2025

fingolfin Feb 28, 2025

Choose a reason for hiding this comment

ThomasBreuer Apr 1, 2025

Choose a reason for hiding this comment

fingolfin Mar 31, 2025

Choose a reason for hiding this comment

fingolfin Mar 31, 2025

Choose a reason for hiding this comment

fingolfin left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 6, 2025 •

edited

Loading