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

kRegularSequenceSpace.from_recurrence: make offset keyword-only #32198

Closed
galipnik opened this issue Jul 13, 2021 · 20 comments
Closed

kRegularSequenceSpace.from_recurrence: make offset keyword-only #32198

galipnik opened this issue Jul 13, 2021 · 20 comments

Comments

@galipnik
Copy link

In view of #31787, offset should be a keyword-only argument of kRegularSequenceSpace.from_recurrence and RecurrenceParser.__call__.

Depends on #27940

CC: @cheuberg @dkrenn

Component: combinatorics

Author: Gabriel F. Lipnik

Branch/Commit: 72bd352

Reviewer: Clemens Heuberger, Daniel Krenn

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

@galipnik galipnik added this to the sage-9.4 milestone Jul 13, 2021
@galipnik
Copy link
Author

Branch: u/galipnik/k-regular-offset

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 14, 2021

Commit: dd8b09f

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 14, 2021

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

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
dd8b09fTrac #32198: make offset keyword-only

@galipnik

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 14, 2021

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

c64cd60Trac #32198: mention positional and keyword-only arguments in docstring

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 14, 2021

Changed commit from dd8b09f to c64cd60

@cheuberg
Copy link
Contributor

Reviewer: Clemens Heuberger

@cheuberg
Copy link
Contributor

comment:6

LGTM. Let us wait a few days to give dkrenn a chance to have a look on it and then feel free to set to positive unless dkrenn objects.

@dkrenn
Copy link
Contributor

dkrenn commented Jul 18, 2021

comment:7

Why do we need make offset explicit in

-    def from_recurrence(self, *args, **kwds):
+    def from_recurrence(self, *args, offset=0):

? Shouldn't it suffices (as done) in

+    def __call__(self, *args, offset=0):

Apart from this, changes LGTM

@dkrenn
Copy link
Contributor

dkrenn commented Jul 18, 2021

Changed reviewer from Clemens Heuberger to Clemens Heuberger, Daniel Krenn

@dkrenn
Copy link
Contributor

dkrenn commented Jul 18, 2021

comment:10

Replying to @dkrenn:

Why do we need make offset explicit in

-    def from_recurrence(self, *args, **kwds):
+    def from_recurrence(self, *args, offset=0):

?

Briefly tested using **kwds (on #31787); seems to work.

@mkoeppe
Copy link
Member

mkoeppe commented Jul 19, 2021

comment:11

Setting a new milestone for this ticket based on a cursory review.

@mkoeppe mkoeppe modified the milestones: sage-9.4, sage-9.5 Jul 19, 2021
@dkrenn
Copy link
Contributor

dkrenn commented Jul 27, 2021

Changed branch from u/galipnik/k-regular-offset to u/dkrenn/k-regular-offset

@dkrenn
Copy link
Contributor

dkrenn commented Jul 27, 2021

comment:13

Replying to @dkrenn:

Replying to @dkrenn:

Why do we need make offset explicit in

-    def from_recurrence(self, *args, **kwds):
+    def from_recurrence(self, *args, offset=0):

?

Briefly tested using **kwds (on #31787); seems to work.

Changed.


New commits:

72bd352Trac #32198: pass on all kwds (instead of "offset" explicitly)

@dkrenn
Copy link
Contributor

dkrenn commented Jul 27, 2021

Changed commit from c64cd60 to 72bd352

@dkrenn dkrenn modified the milestones: sage-9.5, sage-9.4 Jul 27, 2021
@galipnik
Copy link
Author

comment:16

Replying to @dkrenn:

Changed.

Thank you, commit 72bd352 LGTM.

@dkrenn
Copy link
Contributor

dkrenn commented Jul 27, 2021

comment:17

Patchbot is happy :)

@cheuberg
Copy link
Contributor

comment:18

commit 72bd352 LGTM.

@slel
Copy link
Member

slel commented Jul 27, 2021

comment:19

Setting to critical in view of

@mkoeppe mkoeppe modified the milestones: sage-9.4, sage-9.5 Aug 22, 2021
@vbraun
Copy link
Member

vbraun commented Aug 29, 2021

Changed branch from u/dkrenn/k-regular-offset to 72bd352

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

6 participants