kernel: fix bug in LtTrans24 in trans.c

[james-d-mitchell]

Please make sure that this pull request:

• is submitted to the correct branch (the stable branch is only for bugfixes)
• contains an accurate description of changes for the release notes below
• provides new tests or relies on existing ones
• correctly refers to other issues and related pull requests

Tick all what applies to this pull request

• Adds new features
• Improves and extends functionality
• Fixes bugs that could lead to crashes
• Fixes bugs that could lead to incorrect results
• Fixes bugs that could lead to break loops

Write below the description of changes (for the release notes)

This PR fixes a bug in LtTrans24, a for loop was incorrectly nested.
In order to increase the test coverage to include this part of the code
(and a number of other parts of the code in trans.c that were previously
unreachable) IdentityTrans is altered to be a T_TRANS4. Tests are added
for this part of the code, and the other previously unreachable parts.

This resolves Semigroups Issue 251.

[ChrisJefferson]

Won't it make code less efficient if IdentityTrans is a T_TRANS4, as code using IdentityTrans will now head down the wrong path?

If you don't want to force all trans under degree 65,536 to be T_TRANS2, then perhaps we could add a simple function which would let you explicitly turn T_TRANS2 into a T_TRANS4, which we could then use for testing?

[codecov]

Codecov Report

Merging #1130 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1130      +/-   ##
==========================================
+ Coverage   59.19%   59.24%   +0.04%     
==========================================
  Files         434      434              
  Lines      231093   231096       +3     
==========================================
+ Hits       136806   136908     +102     
+ Misses      94287    94188      -99

Impacted Files	Coverage Δ
src/trans.c	99.8% <100%> (+3.47%)	✅
src/compiler.c	65.5% <ø> (+0.04%)	✅
src/permutat.c	55.21% <ø> (+0.15%)	✅
src/system.c	63.88% <ø> (+1.04%)	✅

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb6d4d1...f8c4f0c. Read the comment docs.

[james-d-mitchell]

@ChrisJefferson I don't see how this could have any effect on performance. The performance of most (all?) of the functions in trans.c is a function of the (internal) degree of the transformations which are the arguments, so if IdentityTrans is one of those arguments, its degree is 0.

I might be overlooking something, can you point me to an example in the code of what you were thinking of?

I like the idea of having a function that converts from T_TRANS2 to T_TRANS4, this would make the tests clearer. Is it worth having such a function just for testing?

[flsmith]

This bug is causing me some headaches, and I would appreciate seeing a fix merged.

[ChrisJefferson]

I'm worried by the tests, as far as I can tell the only transformation we ever test with is the identity transformation -- it would be easy for there to be a bug where the identity transformation worked, but almost anything else didn't.

[james-d-mitchell]

@ChrisJefferson if you look at the tests, you'll see that they do not only test that things work for the identity transformation. Having IdentityTrans in trans.c be a T_TRANS4 means that we can create arbitrary transformations of type T_TRANS4 by multiplying the output of (say) AS_TRANS_PERM_INT((), 0) by any other transformation.

Previously none of the blocks of code for transformations of type T_TRANS4 with (internal) degree less than 65536 were tested at all.

[james-d-mitchell]

@ChrisJefferson would you prefer if I removed the tests and the change of IdentityTrans to a T_TRANS4 and made a PR that fixes the bug in the LtTrans24, or what?

[markuspf]

You could just introduce a separate object for the purposes of testing the T_TRANS4 branches (which is not that Identity transformation in trans.c.

[ChrisJefferson]

When you say multiplying anything by the identity transformation gives a TRANS_4 now, that seems like an example of inefficiency caused by this patch :)

However, for the purposes of getting this committed in, how about adding an explicit new global ID_TRANS_4 (not fixed on the name), which is an identity transformation on TRANS_4, then you can use that in tests and not effect the built-in identity transformation?

[james-d-mitchell]

@ChrisJefferson @markuspf Yes, I see what you mean, I will introduce ID_TRANS_4, and use this in place of what I did before. This is a better idea than what I did here.

[james-d-mitchell]

@ChrisJefferson @markuspf I've made the suggested changes, should be good to go.

[markuspf]

Lets wait for the tests to go through and then we're good to go.

[ChrisJefferson]

I'm happy with this now.