Add support for BMat8 by james-d-mitchell · Pull Request #48 · libsemigroups/Semigroups.jl

james-d-mitchell · 2026-02-21T19:47:40Z

No description provided.

james-d-mitchell · 2026-02-21T19:48:31Z

I think all the functionality is there, I just have to finish adding the documentation.

jswent · 2026-02-22T11:18:13Z

@james-d-mitchell I think your fork is behind since the tests are out of sync with main. Just merged in, everything should pass now.

EDIT: Nevermind, hadn't seen that you modified transf. Will do a full review later.

codecov-commenter · 2026-02-22T16:35:58Z

Codecov Report

❌ Patch coverage is 76.47059% with 12 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@4cf738b). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
src/elements/bmat8.jl	76.47%	12 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main      #48   +/-   ##
=======================================
  Coverage        ?   65.33%           
=======================================
  Files           ?       10           
  Lines           ?      450           
  Branches        ?        0           
=======================================
  Hits            ?      294           
  Misses          ?      156           
  Partials        ?        0

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

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

james-d-mitchell · 2026-02-24T10:30:45Z

I just need to add tests and this should be good to go (note everything is tested in the docs, so it's just a matter of extracting the code examples and putting them in a test file).

james-d-mitchell · 2026-02-24T10:30:55Z

@jswent Any idea why the docs are failing?

jswent · 2026-02-24T11:37:21Z

@jswent Any idea why the docs are failing?

not sure, haven't seen a segfault before. just tested on a throwaway PR and got the same result, which was passing before. will look into it later.

jswent · 2026-02-24T14:06:04Z

@james-d-mitchell should be fixed, appears to be a problem with the latest version of julia so I pinned a stable version to the CI checks

james-d-mitchell · 2026-02-24T14:54:14Z

Thanks @jswent!

james-d-mitchell · 2026-02-25T10:40:09Z

This is now good to go

src/libsemigroups/bmat8.jl

deps/src/bmat8.cpp

src/elements/bmat8.jl

james-d-mitchell · 2026-02-26T10:21:02Z

Thanks for the comments @jswent, I've updated the PR accordingly, please let me know if have any further suggestions.

jswent

just a few more notes then should be good to go. tests failing is something different (increase_degree_by upstream changes), so I'll fix that

jswent · 2026-02-26T11:11:53Z

src/elements/transf.jl

this needs to revert back to using Base.one()

jswent · 2026-02-26T11:33:50Z

src/elements/bmat8.jl

+function Base.getindex(x::BMat8, r::Int64, c::Int64)::Bool
+    return @wrap_libsemigroups_call LibSemigroups.at(x, UInt(r - 1), UInt(c - 1))
+end


I think it would be better to keep the index conversion in the C++ bindings layer to maintain consistency. Thoughts?

It might be better to keep everything in LibSemigroups as close to the original C++ as possible, for the purposes of resolving bugs. Not sure though. I've also been generating the bindings for Forest and it requires a fairly large amount of wrangling the parameters, which more or less cannot be done directly in the C++ code, which provides a further rationale for keeping this here.

docs/src/data-structures/elements/matrix/bmat8.md

src/elements/bmat8.jl

src/elements/transf.jl

jswent · 2026-02-26T11:46:14Z

src/elements/bmat8.jl

+function BMat8(rows::Vector{Vector{T}})::BMat8 where {T<:Union{Bool,Int64}}
+    result = BMat8(0)
+    n = length(rows[1])
+    for (i, row) in enumerate(rows)
+        if length(row) != n
+            throw(
+                LibsemigroupsError(
+                    "the entries of the argument (rows) must all be the same length $n, found length $(length(row)) for row with index $i",
+                ),
+            )
+        end
+        result[i] = row
+    end
+    return result
+end


Possibly worth adding some extra validation here based on what we discussed yesterday (BMat8 iterates and writes rows one-by-one) as currently behaves like:

julia> x = BMat8(0) julia> for i in 1:9; x[i] = [1, 1]; end # ERROR: LibsemigroupsError("...the 1st argument (row index) must be < 8, found 8") julia> x # rows 1-8 were already written BMat8([[1, 1, 0, 0, 0, 0, 0, 0], [1, 1, 0, 0, 0, 0, 0, 0], [1, 1, 0, 0, 0, 0, 0, 0], [1, 1, 0, 0, 0, 0, 0, 0], [1, 1, 0, 0, 0, 0, 0, 0], [1, 1, 0, 0, 0, 0, 0, 0], [1, 1, 0, 0, 0, 0, 0, 0], [1, 1, 0, 0, 0, 0, 0, 0]])

possibly something like

Suggested change

function BMat8(rows::Vector{Vector{T}})::BMat8 where {T<:Union{Bool,Int64}}

result = BMat8(0)

n = length(rows[1])

for (i, row) in enumerate(rows)

if length(row) != n

throw(

LibsemigroupsError(

"the entries of the argument (rows) must all be the same length $n, found length $(length(row)) for row with index $i",

),

)

end

result[i] = row

end

return result

end

function BMat8(rows::Vector{Vector{T}})::BMat8 where {T<:Union{Bool,Int64}}

if length(rows) > 8

throw(LibsemigroupsError(

"expected the argument (rows) to have at most 8 rows, found $(length(rows))"))

end

result = BMat8(0)

n = length(rows[1])

if n > 8

throw(LibsemigroupsError(

"expected rows in the argument (rows) to have length at most 8, found $n"))

end

for (i, row) in enumerate(rows)

if length(row) != n

throw(

LibsemigroupsError(

"the entries of the argument (rows) must all be the same length $n, found length $(length(row)) for row with index $i",

),

)

end

result[i] = row

end

return result

end

not sure how you would feel about this?

probably better to apply these changes yourself since i wrote this in the browser (not sure if formatting is correct)

Possibly worth adding some extra validation here based on what we discussed yesterday (BMat8 iterates and writes rows one-by-one) as currently behaves like:

julia> x = BMat8(0)
julia> for i in 1:9; x[i] = [1, 1]; end

ERROR: LibsemigroupsError("...the 1st argument (row index) must be < 8, found 8")

julia> x # rows 1-8 were already written
BMat8([[1, 1, 0, 0, 0, 0, 0, 0],
[1, 1, 0, 0, 0, 0, 0, 0],
[1, 1, 0, 0, 0, 0, 0, 0],
[1, 1, 0, 0, 0, 0, 0, 0],
[1, 1, 0, 0, 0, 0, 0, 0],
[1, 1, 0, 0, 0, 0, 0, 0],
[1, 1, 0, 0, 0, 0, 0, 0],
[1, 1, 0, 0, 0, 0, 0, 0]])

As far as I can tell this does exactly as I'd expect, the error only occurs when i = 9 so the output is as expected. I think I added the correct amount of argument checking, everything else is checked in libsemigroups. If more argument checks are required we should add them in libsemigroups itself and not here. Which makes me think I should add the checks here to libsemigroups too (if they're not there already).

jswent · 2026-02-26T11:49:32Z

src/elements/bmat8.jl

+end
+
+function Base.setindex!(x::BMat8, row::Vector{T}, r::Int64) where {T<:Union{Bool,Int64}}
+    @wrap_libsemigroups_call setindex!(x, Vector{UInt8}(row), r)


I think the same problem as the constructor would happen here, where setting a row with >8 elements writes columns 1-8 successfully and then errors out

jswent · 2026-02-26T11:50:02Z

src/elements/bmat8.jl

+- `rows::Vector{Vector{T}} where {T<:Union{Bool,Int64}}`: the rows of the matrix.
+
+# Throws
+- `MethodError`: if `rows` has `0` rows.


Suggested change

- `MethodError`: if `rows` has `0` rows.

- `BoundsError`: if `rows` has `0` rows.

This throws a MethodError for me:

julia> BMat8([]) ERROR: MethodError: no method matching BMat8(::Vector{Any}) The type `BMat8` exists, but no method is defined for this combination of argument types when trying to construct it. Closest candidates are: BMat8() @ Semigroups none:0 BMat8(::Integer) @ Semigroups none:0 BMat8(::Array{Vector{T}, 1}) where T<:Union{Bool, Int64} @ Semigroups ~/git/Semigroups.jl/src/elements/bmat8.jl:195 Stacktrace: [1] top-level scope @ REPL[1]:1

src/elements/bmat8.jl

jswent · 2026-02-26T21:38:51Z

@james-d-mitchell tests should pass now if you rebase main

Co-authored-by: Jake Swent <94636565+jswent@users.noreply.github.com>

james-d-mitchell marked this pull request as draft February 21, 2026 19:48

james-d-mitchell force-pushed the bmat8 branch from 15955b6 to f29327f Compare February 22, 2026 16:28

james-d-mitchell force-pushed the bmat8 branch from f29327f to d94bd2c Compare February 23, 2026 09:07

james-d-mitchell force-pushed the bmat8 branch from d94bd2c to eada35e Compare February 24, 2026 10:32

james-d-mitchell force-pushed the bmat8 branch from e55711d to 4a9d1d3 Compare February 25, 2026 10:39

james-d-mitchell marked this pull request as ready for review February 25, 2026 10:39

james-d-mitchell force-pushed the bmat8 branch 2 times, most recently from 9ef5e1c to f91e6d9 Compare February 25, 2026 15:16

jswent requested changes Feb 25, 2026

View reviewed changes

src/libsemigroups/bmat8.jl Outdated Show resolved Hide resolved

deps/src/bmat8.cpp Outdated Show resolved Hide resolved

src/elements/bmat8.jl Outdated Show resolved Hide resolved

src/elements/bmat8.jl Show resolved Hide resolved

src/elements/bmat8.jl Outdated Show resolved Hide resolved

james-d-mitchell force-pushed the bmat8 branch from f91e6d9 to 95c950d Compare February 26, 2026 10:20

james-d-mitchell force-pushed the bmat8 branch from 95c950d to cfee39a Compare February 26, 2026 10:23

jswent requested changes Feb 26, 2026

View reviewed changes

james-d-mitchell added 4 commits March 3, 2026 08:43

make scripts/generate_jlcxx.py executable

9139c6e

runner: fix reference in contents table

4a4e6c9

Add support for BMat8

077782d

Updates from code review

f656bbd

james-d-mitchell force-pushed the bmat8 branch from cfee39a to f656bbd Compare March 3, 2026 08:43

james-d-mitchell and others added 2 commits March 3, 2026 12:06

Apply suggestions from code review

2e6bae9

Co-authored-by: Jake Swent <94636565+jswent@users.noreply.github.com>

Update docs/src/data-structures/elements/matrix/bmat8.md

3df8c85

Co-authored-by: Jake Swent <94636565+jswent@users.noreply.github.com>

Update src/elements/transf.jl

5de5886

Co-authored-by: Jake Swent <94636565+jswent@users.noreply.github.com>

james-d-mitchell merged commit fdbba42 into libsemigroups:main Mar 6, 2026
11 checks passed

	- `MethodError`: if `rows` has `0` rows.
	- `BoundsError`: if `rows` has `0` rows.

Conversation

james-d-mitchell commented Feb 21, 2026

Uh oh!

james-d-mitchell commented Feb 21, 2026

Uh oh!

jswent commented Feb 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Feb 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

james-d-mitchell commented Feb 24, 2026

Uh oh!

james-d-mitchell commented Feb 24, 2026

Uh oh!

jswent commented Feb 24, 2026

Uh oh!

jswent commented Feb 24, 2026

Uh oh!

james-d-mitchell commented Feb 24, 2026

Uh oh!

james-d-mitchell commented Feb 25, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

james-d-mitchell commented Feb 26, 2026

Uh oh!

jswent left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ERROR: LibsemigroupsError("...the 1st argument (row index) must be < 8, found 8")

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jswent commented Feb 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jswent commented Feb 22, 2026 •

edited

Loading

codecov-commenter commented Feb 22, 2026 •

edited

Loading