search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.44k stars 481 forks source link

get k-regular sequence from certain recurrence relations #27940

Closed galipnik closed 3 years ago

galipnik commented 5 years ago

Code for constructing the linear representation of k-regular sequences given by certain recurrence relations.

See also Meta ticket #21202.

Depends on #21295 Depends on #21203

CC: @dkrenn

Component: combinatorics

Author: Gabriel F. Lipnik, Daniel Krenn

Branch/Commit: 488123c

Reviewer: Clemens Heuberger, Daniel Krenn

Issue created by migration from https://trac.sagemath.org/ticket/27940

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

b3d2cbefix identation of docstring
6bd41cdadd further doctests
8f53887some further improvements
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 96fbad8 to 8f53887

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

1d9a42crename k to j in some indices
c63d08efurther minor improvements
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 8f53887 to c63d08e

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

92ef82fremove redundant arguments of _get_matrix_
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from c63d08e to 92ef82f

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 92ef82f to d9a5322

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

d9a5322recursion --> recurrence
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

9ade52bfurther doctests
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from d9a5322 to 9ade52b

cheuberg commented 3 years ago
comment:36

I rebased the branch such that it only contains material from the dependencies #21295, #21203 plus the changes pertinent to this ticket.

More precisely, I rebased commits from 4fe100c576 to eb44f15e6e onto #21203 and then commits from 0b039048e6 until the current branch u/galipnik/k-regular-recursions on top of that.

@galpnik: Please cross-review this rebase.

I will review your changes after that.

Last 10 new commits:

d0676a8simplify _get_matrix_from_recurrence_
430d0f2add negative initial values
85e2200fix identation of docstring
3f074feadd further doctests
15a3607some further improvements
4e33084rename k to j in some indices
0d6707cfurther minor improvements
72d8e32remove redundant arguments of _get_matrix_
642b4d3recursion --> recurrence
fa9bed3further doctests
cheuberg commented 3 years ago

Changed commit from 9ade52b to fa9bed3

cheuberg commented 3 years ago

Changed branch from u/galipnik/k-regular-recursions to u/cheuberg/k-regular-recursions-rebased

galipnik commented 3 years ago

Changed branch from u/cheuberg/k-regular-recursions-rebased to u/galipnik/k-regular-recurions-rebased

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

da90fc5Trac #27940: import cached_function again
8225c71Trac #27940: change order of elements in output dictionaries of some tests
61d6c6eTrac #27940: remove superfluous import of ceil
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from fa9bed3 to 61d6c6e

galipnik commented 3 years ago
comment:39

Replying to @cheuberg:

@galpnik: Please cross-review this rebase.

Thank you! Three new commits pushed, the rest LGTM.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 61d6c6e to 43e892e

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

6031c8cTrac #27940: fix pyflakes findings
43e892eTrac #27940: 40: add "from None"
galipnik commented 3 years ago
comment:42

Here is a more detailed answer to your review:

Replying to @cheuberg:

  1. Dependencies: It seems that many more branches are merged here than necessary. Please fill out the dependencies field here.

Done. See also comment:36.

  1. Fix patchbot findings

Done.

  1. recursions: description of the equations parameter: I am not sure whether writing it as sum(...) can cause any confusion. I propose to write some summands with ....

Done and printed in LaTeX.

  1. recursions: description of the equations parameter: I guess you want to include some coefficients in front of the terms on the right hand side.

Done.

  1. recursions: dead link to minimized() in description of parameter minimize

Fixed.

  1. _parse_recursions_: catch AttributeError when a non-element of the symbolic ring is given as an equation: Seq2._parse_recursions_([42], f, n)

Done and test case added.

  1. _parse_recursions_ before throwing %s is not a polynomial: I think that explicitly naming the exceptions which are likely to occur in the conversion attempt to a polynomial is preferred over generically catching Exception.

Done.

  1. _parse_recursion_: base_ring[var](left_side.operands()[0]) I am unsure whether base_ring is the correct ring here. After all, arguments of a regular sequence will alwys be the integers. A few lines later, there is ZZ[var](left_side.operands()[0]) which seems to be the ring we need to use. So my suggestion is to always use ZZ[var] and polynomial_left and removing poly_left. Also, polynomial_left in base_ring does not seem to be the correct ring.

Done.

  1. _parse_recursion_: initial_values.update({polynomial_left: right_side}) I suggest to check whether an initial value is repeatedly given.

Done: If the values are different for same n, then an exception is raised.

  1. _parse_recursion_: if M != log(base_power_M, base=k): I suggest to replace the right hand side by M_new which has just been defined one line before.

Done.

  1. _parse_recursion_: if r not in ZZ: raise ValueError("%s is not an integer." % (r,)): I do not think that this can be reached: r is a coefficient of a polynomial over ZZ anyway. In particular, Seq2._parse_recursions_([f(2*n+1/2) == f(n)], f, n) leads to ValueError: 2*n + 1/2 is not a polynomial in n.

Fixed.

  1. _parse_recursion_: else: # check this again I do not understand the comment.

Removed.

  1. _parse_recursion_._parse_multiplication_: I think that if op.operator() != mul_vararg or len(op.operands()) != 2: raise ValueError("") is unreachable, so I suggest to replace this by assert op.operator() == mul_vararg and len(op.operands()) == 2.

Yes, I agree. Done.

  1. _parse_recursion_._parse_one_summand_: raise ValueError('%s does not have integer coefficients.' % (op.operands()[0],)): The error message does not seem to fit all cases of failing conversion to an integer polynomial; e.g., Seq2._parse_recursions_([f(2*n+1) == 2*f(1/n)], f, n) yields `ValueError: 1/n does not have integer coefficients.

I have modified this.

  1. _parse_recursion_._parse_one_summand_: It should be checked that len(op.operands()) == 1; currently, Seq2._parse_recursions_([f(2*n+1) == 2*f(n+4, 5), f(2*n) == f(n)], f, n) does not yield an error.

Exception added.

  1. _parse_recursion_: Branch if not coeffs: As far as I understand, this means that no general equations have been given, only initial values. I do not understand why we do not raise an error in this case.

No, this can also happen for the zero sequence, like f(2*n) == 0, f(2*n + 1) == 0. After refactoring the parsing method, this case is handled in lines 835 and 933 now.

  1. __get_ind_: I would replace the two consecutive .update calls to the same dictionary by one single call .update({(j, d): pos, pos: (j, d)}) (twice)

Done.

  1. _parse_recursion_: docstring: I think that even for a private method, the contents of the namedtuple should be explained.

Done.

  1. _get_ind_ docstring: I think that even for a private method, the output should be explained.

Done.

  1. _get_matrix_from_recursions_ docstring: inputs var and function are not explained.

These inputs are not needed anymore.

  1. _get_matrix_from_recursions_ docstring: Please explain what the role of the parameter correct_offset is.

Done.

  1. _get_matrix_from_recursions_ example on the number of unbordered factors in the Thue–Morse sequence: please only put one recurrence equation per line in order to make the recurrences more readable.

Done.

  1. _get_matrix_from_recursions_ example on the number of unbordered factors in the Thue–Morse sequence: why are there so many initial values? If my calculations are correct, then the "largest" component of the linear representation corresponds to the sequence f(4n+9); for n=3, this is f(21). It might be that this large number of initial values is required in the current code, but I think that the original recurrences could and should be used to compute as many auxiliary values as required. This means that only initial values up to 15 should be required.

New method _get_values_from_recurrence_ has been implemented now. Consequently, only values up to f(23) are needed.

  1. _get_matrix_from_recursions_: the line n1 = recursion_rules.n1 appears twice.

Removed.

  1. _get_matrix_from_recursions_: I think that the lines which define temp twice in different ways are rather hard to read. If (as suggested in 22) the required auxiliary values are computed from the initial values in a separate method, then I imagine that these lines could be simply replaced by suitable list comprehensions. The symbolic vector arguments could be replaced by a function taking an argument and returning a vector, so no substitutions would be required. Then it might not be necessary to have function and var as parameters of this method.

New method _v_eval_n_from_recurrence_ has been implemented now. Consequently, _get_matrix_from_recursions_ is much simpler now.

  1. _get_matrix_from_recursions_: the condition for construction the matrix J could be simplified: the conditions i>= rem and i % k == rem can be omitted, because they follow from j*k == i-rem. Additionally, it might be simpler to construct this matrix by the version of the matrix constructor which takes a function as an argument.

I removed the redundant conditions. I do not understand the suggestion in your last sentence.

  1. _get_right_from_recursions_: The same comment about initial values as mentioned in 22 applies. If a separate method for computing the vector of the linear representation for given arguments is implemented as proposed in 24, then this method here could be shortened considerably. The parameter function might then be redundant.

Done.

  1. recursions: example Stern--Brocot: The initial value f(2) should not be required.

Removed.

  1. recursions: example Odd Entries in Pascal's Triangle: only f(0) and f(1) should be required as initial values.

Removed.

  1. recursions: example Unbordered Factors: one equation per line (cf. 21)

Done.

  1. recursions: example Unbordered Factors: initial values (cf. 22)

Done.

  1. recursions: docstring: "in the a Generalized" remove "a"

Done.

  1. recursions: "[TODO: reference]" please resolve TODO.

Done.

  1. recursions: example Generalized Pascal's Triangle: only initial values f(0) and f(1) should be required.

Done.

  1. recursions: mu could be constructed as a list comprehension instead of appending to a list.

Done.

  1. This ticket adds quite a number of private methods to kRegularSequenceSpace which are only useful for the public method recursions. For clarity's sake, I propose to introduce a "see also" block in all auxiliary private methods which refers to recursions.

Done.

  1. The method _get_ind_ should be renamed such that it clearly relates to the main public method (this is now the case for all other private methods except this one).

Done.

  1. I am not yet convinced that kRegularSequenceSpace.recursions is a good description of the method. Wouldn't one expect to get recursions by such a method, instead of getting a k-regular sequence from a recursion? Perhaps from_recursion or possibly more precise from_recurrence would be a better name.

Changed to from_recurrence.

  1. Please extend the docstring of recursions in such a way that a clearer link is provided to [HKL2021]. For instance, the docstring could state that such recurrence relations uniquely define a k regular sequence (provided that enough initial values are provided).

Done.

  1. Test raising "Initial value ... is missing".

This exception is removed now, because the method _get_values_from_recurrence_ checks this.

  1. Consider using raise ValueError(...) from None (once this code runs under python3) in order to supress printing of the detailed exceptions which are explained by the ValueError of this code here.

Done.

cheuberg commented 3 years ago
comment:44

Thank you for your changes and comments.

One reply and a few more comments which came up while reading your changes.

  1. _get_matrix_from_recursions_ [...] Additionally, it might be simpler to construct this matrix by the version of the matrix constructor which takes a function as an argument.

[...]. I do not understand the suggestion in your last sentence.

It meant rewriting it as follows:

def entry(i, kk):
    j, d = ind [i]
    if j < M - 1:
        return int(kk == ind[(j + 1, k**j*rem + d)] - 1)
    else:
        rem_d = k**(M-1)*rem + (d%k**M)
        dd = d // k**M
        if rem_d < k**M:
            lambd = l - ind[(m, (k**m)*dd + l)]
            return _coeff_(rem_d, kk + lambd)
        else:
            lambd = l - ind[(m, k**m*dd + k**m + l)]
            return _coeff_(rem_d - k**M, kk + lambd)

mat = Matrix(base_ring, dim_without_corr, dim_without_corr, entry)

  1. Naming of the variable in docstrings. In some docstrings, we have "a symbolic variable n" or similar formulations. I think that we should not make any assumptions about the name of the variable, it is simply a variable. There might be an exception in those docstrings where equations are parsed, there I would say "symbolic variable (n in the above description of equations).

  2. Naming of the variable in exceptions: Some exceptions have a hard-coded n. I think that it would be more helpful to use the variable name provided by the user. It might also be good to have one test with different names for function and variable.

  3. _get_right_from_recurrence_: Docstring: Please move the "see also" block before the "tests" block (I think that we should adhere to the order of the blocks as described in https://doc.sagemath.org/html/en/developer/coding_basics.html#the-docstring-of-a-function-content)

  4. from_recurrence: Please document parameter offset explicitly (e.g. "an integer (default: 0). See explanation in equations above")

  5. _get_values_from_recurrence_: if _coeff_(r, j) != 0 could be replaced by if _coeff_(r, j)

  6. _get_values_from_recurrence_: the check f_n not in base_ring could come earlier (when checking the provided parameters, not (possibly repeatedly) on usage

  7. _parse_recurrence_: docstring: replace "vector" by "tuple"

  8. _parse_recurrence_: why is remainders a list instead of a set?

  9. _parse_recurrence_: elif M and not m: I think that this should be replaced by elif M and m is None:: Otherwise, I fear that legitimate situations such as M=3, m=0 with a bunch of equations might be erroneously corrected to M=3, m=2.

  10. _get_parameters_from_recurrence_: offset = max(0, -l/k**m): possibly take ceiling of -l/k**m to have an integer offset?

  11. In the old code, there were two tests

sage: Seq2._parse_recurrence_([f(2*n) == f(n)], f, n)
Traceback (most recent call last):
...
ValueError: Recurrence relations for [f(2*n + 1)] are missing.

sage: Seq2._parse_recurrence_([f(4*n) == f(n), f(4*n + 3) == 0], f, n)
Traceback (most recent call last):
...
ValueError: Recurrence relations for [f(4*n + 1), f(4*n + 2)] are missing.
which seem not to have survived the refactoring. If I am not mistaken, the corresponding checks are no longer there.

  1. I suggest to always call _get_values_from_recurrence_ with named parameters (there is now quite a number of parameters and we might want to avoid future mistakes). If rewriting the tests is not too costly, we might even enforce that with a * (PEP 3102).

  2. _v_eval_n_from_recurrence: would it be possible to simplify that method by using _get_ind_from_recurrence_, also guaranteeing consistency between the two methods?

  3. A number of docstring has a description like "A namedtuple generated by _parse_recurrence_()". However, this nametuple is no longer generated by _parse_recurrence_, but by _get_parameters_from_recurrence_.

  4. _get_right_from_recurrence_: It seems that the last line return vector(right) could be replaced by return right because all code paths seem to guarantee that right is already a vector.

  5. _get_ind_from_recurrence_: The description of the input parameters does not match the signature of the method.

  6. _get_matrix_from_recurrence_: input parameters function, var are explained, but not part of the signature.

  7. _get_parameters_from_recurrence_: description of namedtuple misses offset.

  8. _v_eval_n_from_recurrence_: missing output section in docstring.

  9. from_recurrence: minimize: The same argument which led to the removal of transpose=True in #21295 (Review item 6) is also applicable here.

  10. There is a number of ReSt-issues because constructions like ``n``th and `i`th are not translated correctly and lead to "WARNING: Inline interpreted text or phrase reference start-string without end-string.". I suggest to insert hyphens in these situations.

  11. In lines 486 and 493, ....: should be replaced by ...

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 43e892e to de1d901

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

e42a389Trac #27940: review item 54: replace `_parse_recurrence_` by
aadf467Trac #27940: add k again...
7aacbc2Trac #27940: review item 55: remove conversion to vector
8d52eb7Trac #27940: review item 56: correct input block
fd678d4Trac #27940: review item 57: remove function and var from input block
2cbde29Trac #27940: review item 58: add offset in output block
d14f0c1Trac #27940: review item 59: add ouput block
5aaf111Trac #27940: review item 60: remove minimize-option
ec7a722Trac #27940: review item 62: colons removed
de1d901Trac #27940: review item 61: fix "n-th" and "i-th"
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

1d7b158Trac #27940: some modifications in the docstrings
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from de1d901 to 1d7b158

galipnik commented 3 years ago
comment:47

Replying to @cheuberg:

Again thank you very much for your review!

Here are my answers and some comments:

  1. [...]

It meant rewriting it as follows: [...]

Done (and fixed the some indices in your suggestion because ind is 1-based), thank you.

  1. Naming of the variable in docstrings. In some docstrings, we have "a symbolic variable n" or similar formulations. I think that we should not make any assumptions about the name of the variable, it is simply a variable. There might be an exception in those docstrings where equations are parsed, there I would say "symbolic variable (n in the above description of equations).

Done.

  1. Naming of the variable in exceptions: Some exceptions have a hard-coded n. I think that it would be more helpful to use the variable name provided by the user. It might also be good to have one test with different names for function and variable.

I'm not sure whether this is a good idea with respect to #31787: If alternative input parameters will be implemented in the future, the user might not provide a variable name. Therefore, I've rephrased the exception messages without using any variable name.

  1. _get_right_from_recurrence_: Docstring: Please move the "see also" block before the "tests" block (I think that we should adhere to the order of the blocks as described in https://doc.sagemath.org/html/en/developer/coding_basics.html#the-docstring-of-a-function-content)

Done.

  1. from_recurrence: Please document parameter offset explicitly (e.g. "an integer (default: 0). See explanation in equations above")

Done.

  1. _get_values_from_recurrence_: if _coeff_(r, j) != 0 could be replaced by if _coeff_(r, j)

Done.

  1. _get_values_from_recurrence_: the check f_n not in base_ring could come earlier (when checking the provided parameters, not (possibly repeatedly) on usage

Done.

  1. _parse_recurrence_: docstring: replace "vector" by "tuple"

Done.

  1. _parse_recurrence_: why is remainders a list instead of a set?

What would be the advantage of a set here?

  1. _parse_recurrence_: elif M and not m: I think that this should be replaced by elif M and m is None:: Otherwise, I fear that legitimate situations such as M=3, m=0 with a bunch of equations might be erroneously corrected to M=3, m=2.

Fixed. The same issue also occured for m (lines 848 and 851), also this should be fixed now. Two additional tests added.

  1. _get_parameters_from_recurrence_: offset = max(0, -l/k**m): possibly take ceiling of -l/k**m to have an integer offset?

Done.

  1. In the old code, there were two tests
sage: Seq2._parse_recurrence_([f(2*n) == f(n)], f, n)
Traceback (most recent call last):
...
ValueError: Recurrence relations for [f(2*n + 1)] are missing.

sage: Seq2._parse_recurrence_([f(4*n) == f(n), f(4*n + 3) == 0], f, n)
Traceback (most recent call last):
...
ValueError: Recurrence relations for [f(4*n + 1), f(4*n + 2)] are missing.
which seem not to have survived the refactoring. If I am not mistaken, the corresponding checks are no longer there.

Fixed again.

  1. I suggest to always call _get_values_from_recurrence_ with named parameters (there is now quite a number of parameters and we might want to avoid future mistakes). If rewriting the tests is not too costly, we might even enforce that with a * (PEP 3102).

Done.

  1. _v_eval_n_from_recurrence: would it be possible to simplify that method by using _get_ind_from_recurrence_, also guaranteeing consistency between the two methods?

Done.

  1. A number of docstring has a description like "A namedtuple generated by _parse_recurrence_()". However, this nametuple is no longer generated by _parse_recurrence_, but by _get_parameters_from_recurrence_.

Fixed.

  1. _get_right_from_recurrence_: It seems that the last line return vector(right) could be replaced by return right because all code paths seem to guarantee that right is already a vector.

Done.

  1. _get_ind_from_recurrence_: The description of the input parameters does not match the signature of the method.

Fixed.

  1. _get_matrix_from_recurrence_: input parameters function, var are explained, but not part of the signature.

Removed.

  1. _get_parameters_from_recurrence_: description of namedtuple misses offset.

Added.

  1. _v_eval_n_from_recurrence_: missing output section in docstring.

Added.

  1. from_recurrence: minimize: The same argument which led to the removal of transpose=True in #21295 (Review item 6) is also applicable here.

Ok, minimize-option removed.

  1. There is a number of ReSt-issues because constructions like ``n``th and `i`th are not translated correctly and lead to "WARNING: Inline interpreted text or phrase reference start-string without end-string.". I suggest to insert hyphens in these situations.

Resolved.

  1. In lines 486 and 493, ....: should be replaced by ...

Done.

cheuberg commented 3 years ago
comment:49

Thank you for your changes and your comments.

I have a few comments on the previous items and list a few new items (which are only relevant now that we seem to converge soon).

25.

I meant rewriting it as follows: [...]

Done (and fixed the some indices in your suggestion because ind is 1-based), thank you.

When writing done my suggestion yesterday, I was exactly afraid of making such off-by-one-errors. When I first reviewed the ticket, it was not clear to me whether having a 1-based ind has any advantages over having it 0-based. It seems to me that in this usage here, we always use ind[j, k] - 1 when using the map in one direction and always use ind[i+1] in the other direction. So: sorry for the late stage question, but would it not make life simpler to have ind 0-based (because this SageMath code lives in a 0-based world?)

  1. Naming of the variable in exceptions: Some exceptions have a hard-coded n. I think that it would be more helpful to use the variable name provided by the user. It might also be good to have one test with different names for function and variable.

I'm not sure whether this is a good idea with respect to #31787: If alternative input parameters will be implemented in the future, the user might not provide a variable name. Therefore, I've rephrased the exception messages without using any variable name.

This is better, thank you.

  1. _parse_recurrence_: why is remainders a list instead of a set?

What would be the advantage of a set here?

Semantically, a python set seems to be a better fit to what is done here (check whether certain elements are present). Performance is probably not an issue here.

  1. Fixed. The same issue also occured for m (lines 848 and 851), also this should be fixed now. Two additional tests added.

Thank you. One remark, though, when reading the new tests: I think that the casual user might not be very happy with an error message "2 does not equal 1" because it might be difficult to see where the problem comes from. It might help to print the offending equation and the offending term: "Term 'f(2n)' in the equation 'f(8n + 1) == f(2*n)': 2 does not equal 1." Sorry for not thinking about this earlier.

  1. In the old code, there were two tests [...] which seem not to have survived the refactoring. If I am not mistaken, the corresponding checks are no longer there.

Fixed again.

I think that raise ValueError(...) from None should be raise ValueError(...) in these checks because we are not handling another exception here.

56 _get_ind_from_recurrence_: The description of the input parameters does not match the signature of the method.

Fixed.

Please replace u by uu in the description of the input parameters.

  1. In lines 486 and 493, ....: should be replaced by ...

Done. 62

There are still 4 instead of 3 dots.

  1. Please merge the dependency #21203 once more (there seems to be a merge conflict).

  2. Please move references to src/doc/en/reference/references/index.rst as per the developer's guide ​https://doc.sagemath.org/html/en/developer/coding_basics.html#the-docstring-of-a-function-content

  3. Please add an arXiv identifier to reference HKL2021 once it is available.

galipnik commented 3 years ago
comment:50

Replying to @cheuberg:

[...] So: sorry for the late stage question, but would it not make life simpler to have ind 0-based (because this SageMath code lives in a 0-based world?)

Changed now. (The initial motivation for implementing it 1-based was consistency with the corresponding paper, but this is better now.)

  1. _parse_recurrence_: why is remainders a list instead of a set?

What would be the advantage of a set here?

Semantically, a python set seems to be a better fit to what is done here (check whether certain elements are present). Performance is probably not an issue here.

Ok, changed.

Thank you. One remark, though, when reading the new tests: I think that the casual user might not be very happy with an error message "2 does not equal 1" because it might be difficult to see where the problem comes from. It might help to print the offending equation and the offending term: "Term 'f(2n)' in the equation 'f(8n + 1) == f(2*n)': 2 does not equal 1." Sorry for not thinking about this earlier.

Hm, good point. Changed a lot in the error messages now to make the errors clearer.

  1. In the old code, there were two tests [...] which seem not to have survived the refactoring. If I am not mistaken, the corresponding checks are no longer there.

Fixed again.

I think that raise ValueError(...) from None should be raise ValueError(...) in these checks because we are not handling another exception here.

Changed.

56 [...]

Please replace u by uu in the description of the input parameters.

Done.

  1. In lines 486 and 493, [...]

There are still 4 instead of 3 dots.

Sorry, fixed now.

  1. Please merge the dependency #21203 once more (there seems to be a merge conflict).

Done and conflict resolved.

  1. Please move references to src/doc/en/reference/references/index.rst as per the developer's guide ​https://doc.sagemath.org/html/en/developer/coding_basics.html#the-docstring-of-a-function-content

Done.

  1. Please add an arXiv identifier to reference HKL2021 once it is available.

Will be done as soon as the paper is on arXiv.

galipnik commented 3 years ago

Changed author from Gabriel Lipnik to Gabriel F. Lipnik

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

5e8b236Trac #21203 review issue 1: better binary sum of digits
a6a8c3bTrac #21203 review issue 2: extend odds in Pascal's triangle
be462dcTrac #21203 review issue 3: example for `__getitem__` and __iter__
cbd187fTrac #21295: rename to coefficient_ring
38743c1Merge branch 't/21295/sequences/recognizable' into t/21203/sequences/k-regular
c4bf7aeTrac #21203 review issue 4: rename to coefficient ring
00ad382Merge branch 'u/dkrenn/sequences/k-regular' into u/galipnik/k-regular-recurions-rebased
acf0c45Trac #27940: move references to index.rst
00f5ad3Trac #27940: add "F." to my name...
3d13b96Trac #27940: fix identation error from merge
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 1d7b158 to 3d13b96

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 3d13b96 to 2c36297

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

2c36297Trac #27940: remove assignment of base_ring
cheuberg commented 3 years ago
comment:54

Thank you for your changes.

I have no further comments, so the only open issues are

  1. arXiv identifier once available
  2. wait for dependency #21203
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

59142caTrac #27940: add arXiv identifier
22e2056Trac #27940: review item 42: add test with different variable and function names
288995eTrac #27940: add name to license
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 2c36297 to 288995e

galipnik commented 3 years ago
comment:56

Three new commits pushed.

  1. [...] It might also be good to have one test with different names for function and variable.

Done. (Note that the comparison for kRegularSequence is implemented in #21319, which is no dependency of this ticket.)

  1. arXiv identifier once available

Done.

dkrenn commented 3 years ago

Changed branch from u/galipnik/k-regular-recurions-rebased to u/dkrenn/k-regular-recurions-rebased

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

cb590eeTrac #27940 move commit: move from_recurrence docstring up
9cc888dTrac #27940: restructure and bring code running
6ff29baTrac #27940: fix doctests (use RecurrenceParser)
22cb3f0Trac #27940: remove underscores from helper methods
293905aTrac #27940: remove "_from_recurrence" from method names
75fe130Trac #27940: remove "get_" from method names
952d724Trac #27940: consistently use coefficient_ring
fbf3a81Trac #27940: adapt/complete docstrings
16de2abTrac #27940: unify input parameters
86c5df7Trac #27940: fix indention (due to renamings)
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 288995e to 86c5df7

dkrenn commented 3 years ago

Changed author from Gabriel F. Lipnik to Gabriel F. Lipnik, Daniel Krenn

dkrenn commented 3 years ago
comment:59

As discussed, I've slightly refactored the code to have the parser and all helper methods in a separate class. I suggest to review the changes above commit-wise.

dkrenn commented 3 years ago

Changed reviewer from Clemens Heuberger to Clemens Heuberger, Daniel Krenn

cheuberg commented 3 years ago
comment:60

I reviewed the refactored code, fine for me except for one suggestion:

  1. RecurrenceParser.__call__: I suggest to keep the signature of the output consistent with the signature of the input of k-regular sequences, i.e., (mu, left, right). Otherwise, I think that the lines

    left, mu, right = RP(*args, **kwds)
return self(mu, left, right)

    in kRegularSequenceSpace.from_recurrence are surprising.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 86c5df7 to 6db5f58

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Branch pushed to git repo; I updated commit sha1. New commits:

6db5f58Trac #27940 review 67: change signature of output of RecurrenceParser.__call__
dkrenn commented 3 years ago
comment:62

Replying to @cheuberg:

I reviewed the refactored code, fine for me except for one suggestion:

  1. RecurrenceParser.__call__: I suggest to keep the signature of the output consistent with the signature of the input of k-regular sequences, i.e., (mu, left, right). Otherwise, I think that the lines

    left, mu, right = RP(*args, **kwds)
return self(mu, left, right)

    in kRegularSequenceSpace.from_recurrence are surprising.

Changed. (Note that RecognizableSeries.linear_representation uses left, mu, right; if this should be changed as well (on a follow-up ticket to the whole k-regular stuff #21202), then now would be a good time as it is not yet merged in a stable release, only in a beta. Any thoughts?)

cheuberg commented 3 years ago
comment:63

Yes, in the last iteration of the other ticket (#21238), I was also thinking about the inconsistency between the input of k-regular sequences and the output of linear_representation.

It seems that the order left, mu, right for linear_representation has not been invented within this series of tickets, but is defined in that way at least somewhere in the literature (e.g. Berstel and Reutenauer p. 10). So in my opinion if we aim for consistency, then the signature of element creation in regular sequences should be changed. And, as you say, if it is done, then it should be done very soon.

cheuberg commented 3 years ago
comment:64

As discussed earlier off-site: linear_representation should stick to the ordering in the literature (left, matrices, right); construction of recognizable series should stick to the current implementatoin (matrices, left, right): if we ever decide to make left and right optional, this ordering is required.

This means that we do not change anything here; we just wait for the patchbot.

cheuberg commented 3 years ago
comment:65

@galipnik: could you please also have a look on dkrenn's changes?