Fixes error causing period function in BinaryRecurrenceSequence to stall and added enhancement

[Noel-Roemmele]

Fixes #38112. Fixed issue causing period function in BinaryRecurrenceSequence to stall. In the loop to calculate the period for the prime powers it now raises an error if you loop too many times. Added an enhancement to calculate the period of a not purely period BinaryRecurrenceSequence. To do this it basically just ignores the first m^2 terms and then calculates the period as normal.

📝 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 and checked the documentation preview.

⌛ Dependencies

[github-actions]

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

[DaveWitteMorris]

The fix is good, but I have some suggestions for the comments.

In line 363, please add " (default: True)" after "boolean", because a docstring should specify the default value of a parameter when there is one.

Also, I think the description of eventual could be clarified:
if True, allow the sequence to be eventually periodic, rather than requiring it to be purely periodic. So n_1 might not be congruent to n_2 modulo period(m) unless n_1 and n_2 are large.

Furthermore, I think this line is over 80 characters, so needs to be broken.

There is an editing error in line 397: "then we ignore then we calculate". Also, there is no need to explain the algorithm that you are using. Maybe just say something like:
"Letting eventual be True allows us to find the period of a sequence that is not purely periodic.::"

The example is more clear if (as in preceding examples) you print out a few of the terms.

sage: T = BinaryRecurrenceSequence(5,12,u0=0,u1=1)
sage: [T(n) for n in range(20)]
[0, 1, 5, 7, 5, 9, 5, 3, 5, 1, 5, 7, 5, 9, 5, 3, 5, 1, 5, 7]

Then T.period(10) gives the error, and T.period(10,eventual=True) gives an answer.

Please also print out a few terms in the final example.

I don't think there is much point in repeating an example as a test, so please change the example to something else (or just delete it).

I think the NOTE block should come before the TESTS, not after.

Immediately after line 436 ("if eventual is True:") is a good place to say a few words about the algorithm. Perhaps something like:
There are only m^2 possible pairs modulo m. Since the numbering of the sequence starts at 0, this implies that the term numbered m^2 must be in periodic part of the sequence. Hence, the sequence starting with the terms numbered m^2 and m^2 + 1 must be purely periodic.

For clarity, perhaps it would be good to explicitly add "eventual=False" in line 439.

I think it would be better if the error message in lines 524-526 gives the actual value of m, instead of the letter "m". You can easily do this by using an f-string in line 525:
f"modulo {m} is not a purely " +

[Noel-Roemmele]

Added fixes suggested by Dave Morris with minor changes. The default value is False not True so that is different. In doctest needs to be [T(n) % 10 for n in range(20)] not [T(n) for n in range(20)] to showcase the period. Moved last example to the test just to have everything in one test. Also made the test look like the example. If the doctest seems redundant it can be removed.

[DaveWitteMorris]

Sorry I wasn't clear. Instead of:

                 return BinaryRecurrenceSequence(self.b, self.c,
                    an[0], an[1]).period(m)
            eventual = False

I meant to suggest:

                return BinaryRecurrenceSequence(self.b, self.c,
                    an[0], an[1], eventual = False).period(m)

Please change this. It makes it more clear that the eventual=False case has been reduced to the eventual=True case.

[DaveWitteMorris]

The documentation failed to build, because of a formatting error. The lines

        - ``eventual`` -- boolean (default: `False`); if `True`, allow the
        sequence to be eventually periodic, rather than requiring it to be
        purely periodic. So `n_1` might not be congruent to `n_2` modulo
        `period(m)` unless `n_1` and `n_2` are large.

are a bullet point, so each one after the first needs additional indentation:

        - ``eventual`` -- boolean (default: `False`); if `True`, allow the
          sequence to be eventually periodic, rather than requiring it to be
          purely periodic. So `n_1` might not be congruent to `n_2` modulo
          `period(m)` unless `n_1` and `n_2` are large.

[Noel-Roemmele]

Added more changes suggested by Dave Morris. Fixed the indentation and moved eventual=False to the correct position.

[DaveWitteMorris]

You need double backticks for ``False``, ``True``, and ``period(m)`` in the description of ``eventual`` so that they will be in the correct font in the documentation. I'm sorry I didn't notice this previously.

Similarly, in "Letting `eventual` be `True`..." you need double backticks around ``eventual`` and ``True``.

(You need double backticks around code snippets. Single backticks are for math formulas that should be typeset by latex.)

At the end of this sentence ("...that is not purely periodic.::"), you need to end with either ". ::" (a space between the period and the double colons) or "::" (remove the period). In the documentation, the first one will give a period, and the second one will give a colon, instead of a period. Ending with ".::" prints both a period and a colon, which is not correct. (I didn't realize this until I looked at the documentation that was generated by this PR, and saw that there was a problem.)

Please also merge the latest beta.

[Noel-Roemmele]

Merged the branch with the new beta and added changes to the documentation and doctest suggested by Dave Morris.