Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

get k-regular sequence from certain recurrence relations #27940

Closed
galipnik opened this issue Jun 6, 2019 · 108 comments
Closed

get k-regular sequence from certain recurrence relations #27940

galipnik opened this issue Jun 6, 2019 · 108 comments

Comments

@galipnik
Copy link
Contributor

galipnik commented Jun 6, 2019

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

@galipnik galipnik added this to the sage-8.8 milestone Jun 6, 2019
@embray
Copy link
Contributor

embray commented Jun 14, 2019

comment:2

As the Sage-8.8 release milestone is pending, we should delete the sage-8.8 milestone for tickets that are not actively being worked on or that still require significant work to move forward. If you feel that this ticket should be included in the next Sage release at the soonest please set its milestone to the next release milestone (sage-8.9).

@embray embray removed this from the sage-8.8 milestone Jun 14, 2019
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 12, 2019

Changed commit from 3b7c866 to d367496

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 12, 2019

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

9e0f817Merge tag '8.8' into u/galipnik/k-regular-recursions
0b03904ccMerge tag '8.9' into u/galipnik/k-regular-recursions
d367496n0 as input for _parse_recursions_

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 16, 2019

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

c2c47e0implement correction for n0
7d47dbeadd examples

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 16, 2019

Changed commit from d367496 to 7d47dbe

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 16, 2019

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

c2d9903add another example

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 16, 2019

Changed commit from 7d47dbe to c2d9903

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 16, 2019

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

98c24d1start_values --> initial_values

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 16, 2019

Changed commit from c2d9903 to 98c24d1

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 16, 2019

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

060ca25add doc-strings

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 16, 2019

Changed commit from 98c24d1 to 060ca25

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 17, 2021

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

eb44f15start to implement correction for initial values
6aff6b4Merge branch 'u/galipnik/k-regular-recursions' of git://trac.sagemath.org/sage into u/galipnik/k-regular-recursions

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 17, 2021

Changed commit from 060ca25 to 6aff6b4

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 17, 2021

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

b411bdafix bug from merge
5a7d293add n1 to recursion_rules
b0f0e1bsome fixes for n1 and dim

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 17, 2021

Changed commit from 6aff6b4 to b0f0e1b

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 19, 2021

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

9ced30cadapt tests
0bdf2e1adapt definition of l and u
a29d29badd _get_ind_
b36adacfix constructions

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 19, 2021

Changed commit from b0f0e1b to b36adac

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 20, 2021

Changed commit from b36adac to d0fe273

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 20, 2021

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

d0fe273adapt construction of W and J

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 23, 2021

Changed commit from d0fe273 to 2edf0cf

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 23, 2021

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

49850ebadapt docstring
78e21acmore changes in docstrings
2edf0cfrefactor

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 23, 2021

Changed commit from 2edf0cf to 9b454f9

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 23, 2021

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

9b454f9some more changes in the docstring...

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 23, 2021

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

46fc3e5fix link to :meth:minimized
1e43197some more updates

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 23, 2021

Changed commit from 9b454f9 to 1e43197

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 23, 2021

Changed commit from 288995e to 86c5df7

@dkrenn
Copy link
Contributor

dkrenn commented Jun 23, 2021

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

@dkrenn
Copy link
Contributor

dkrenn commented Jun 23, 2021

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
Copy link
Contributor

dkrenn commented Jun 23, 2021

Changed reviewer from Clemens Heuberger to Clemens Heuberger, Daniel Krenn

@cheuberg
Copy link
Contributor

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 27, 2021

Changed commit from 86c5df7 to 6db5f58

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 27, 2021

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

6db5f58Trac #27940 review 67: change signature of output of RecurrenceParser.__call__

@dkrenn
Copy link
Contributor

dkrenn commented Jun 27, 2021

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

comment:65

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

@galipnik
Copy link
Contributor Author

comment:66

Replying to @cheuberg:

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

Done, the changes are fine for me. Thank you!

@galipnik
Copy link
Contributor Author

galipnik commented Jul 2, 2021

@galipnik
Copy link
Contributor Author

galipnik commented Jul 2, 2021

Changed commit from 6db5f58 to 288995e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 2, 2021

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

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)
6db5f58Trac #27940 review 67: change signature of output of RecurrenceParser.__call__
488123cTrac #27940: remove redundant line

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 2, 2021

Changed commit from 288995e to 488123c

@galipnik
Copy link
Contributor Author

galipnik commented Jul 2, 2021

comment:69

Sorry for the late change, but I incidentally found a redundant line of code and removed it in the last commit 488123c. I've restarted the patchbot.

@cheuberg
Copy link
Contributor

cheuberg commented Jul 4, 2021

comment:70

488123c is fine. Patchbot is happy. Time to set it to positive.

@vbraun
Copy link
Member

vbraun commented Jul 9, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants