Update transf

[james-d-mitchell]

No description provided.

[james-d-mitchell]

Work in progress, just opening a PR for visibility

[jswent]

@james-d-mitchell just added the changes I had committed locally to this PR so it doesn't mess you up. Note that the ! convention in Julia (e.g. increase_degree_by!) indicates a method which mutates its input object in place.

[jswent]

@james-d-mitchell I also moved the docs to a separate pr

[james-d-mitchell]

This error message shouldn't be necessary, there's a check in the c+ code.

[jswent]

@james-d-mitchell I can't seem to find an exception being thrown in the c++ code. I tested the c++ with:

auto t = make<Transf<0, uint8_t>>({0});
std::cout << "Degree before: " << t.degree() << "\n";

t.increase_degree_by(300);  // No exception thrown
std::cout << "Degree after:  " << t.degree() << "\n\n";

std::cout << "t[254] = " << (int)t[254] << " (expected 254)\n";
std::cout << "t[255] = " << (int)t[255] << " (expected 255)\n";
std::cout << "t[256] = " << (int)t[256] << " (expected 256)\n";
std::cout << "t[257] = " << (int)t[257] << " (expected 257)\n";

I get:

Degree before: 1
Degree after:  301

t[254] = 254 (expected 254)
t[255] = 255 (expected 255)
t[256] = 0 (expected 256)
t[257] = 1 (expected 257)

Looking at the Python bindings, they handle this in the Python wrapper layer: https://github.com/libsemigroups/libsemigroups_pybind11/blob/ed041e62327c2a94abe4deba2ccb2773ee0c41df/src/libsemigroups_pybind11/transf.py#L135-L144

which also promotes the type: https://github.com/libsemigroups/libsemigroups_pybind11/blob/ed041e62327c2a94abe4deba2ccb2773ee0c41df/src/libsemigroups_pybind11/transf.py#L145-L149

[jswent]

Is this the desired behavior (degree just wraps around) and type promotion is only done due to Python simplicity, or do we want that here as well?

[james-d-mitchell]

Thanks @jswent, I think:

1. this could be considered a feature or bug in libsemigroups. It could throw an exception if we try to increase the degree beyond what can be represented by the type. On the other hand, the point_type is only the type holding the image values, and it is possible to define a Transf<0, uint8_t> on 300 points, but the values in the transformation will be restricted to the max value storable in a uint8_t. Although this is probably a bit too surprising, and we should turn it into a bug. What say you @jswent and @Joseph-Edwards ?
2. Type promotion in python was just to make Transf more usable in python. Since we don't document the different types of libsemigroups underlying Transf, it shouldn't be necessary to know anything about these inner types when using Transf in python. In Julia we do expose the underlying C++ type, so I think we should simply mirror the C++ behaviour (whatever it ends up being).

Does that make sense, and answer your questions?

[Joseph-Edwards]

In reference to 1., I think describing this as either a feature or a bug is valid, as long as the documentation is accurate.

If we think it's important to be able to represent all possible transformations of a given degree with a given image type, then the above should be considered a bug.

If, instead, we think there's a real use case for having a large number of points mapped to a small number of points, then what we have at the moment is fine.

I think I'm leaning slightly towards the first case.

[james-d-mitchell]

Thanks @Joseph-Edwards, I doubt there is a use case, and I found it surprising, so I'm also leaning towards it being a bug. What say ye @jswent ?

[jswent]

@james-d-mitchell I'm inclined to agree. When first looking at it my intuition was that the degree should be bounded by the scalar max, because otherwise the type can't represent all transformations of a given degree. I feel like having the degree and scalar type be consistent about the domain makes the most sense.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 94.87179% with 2 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@af7566a). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
src/elements/transf.jl	94.44%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main       #3   +/-   ##
=======================================
  Coverage        ?   69.49%           
=======================================
  Files           ?        7           
  Lines           ?      436           
  Branches        ?        0           
=======================================
  Hits            ?      303           
  Misses          ?      133           
  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.

[jswent]

@james-d-mitchell this should be good to merge once checks pass, lmk if you have any suggestions. I'll do perm/pperm in a separate PR tonight or tomorrow.