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

minor details in combinat #32979

Closed
fchapoton opened this issue Dec 5, 2021 · 10 comments
Closed

minor details in combinat #32979

fchapoton opened this issue Dec 5, 2021 · 10 comments

Comments

@fchapoton
Copy link
Contributor

as suggested by lgtm

fix some default mutable values, currently modified in the code

Component: combinatorics

Author: Frédéric Chapoton

Branch/Commit: cee5d62

Reviewer: David Coudert

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

@fchapoton fchapoton added this to the sage-9.5 milestone Dec 5, 2021
@fchapoton
Copy link
Contributor Author

Commit: 759d49d

@fchapoton
Copy link
Contributor Author

New commits:

759d49dsome details in combinat (fix mutable default values)

@fchapoton
Copy link
Contributor Author

Branch: u/chapoton/32979

@dcoudert
Copy link
Contributor

dcoudert commented Dec 5, 2021

comment:2

In parallelogram_polyomino.py, you could do the same for methods:

  • def _to_binary_tree_Aval_Boussicault(self, position=[0, 0]):
  • def _to_tikz_bounce(self, directions=[0, 1]):
  • def _plot_bounce(self, directions=[0,1]):

@dcoudert
Copy link
Contributor

dcoudert commented Dec 5, 2021

Reviewer: David Coudert

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 5, 2021

Changed commit from 759d49d to cee5d62

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 5, 2021

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

cee5d62fixes

@fchapoton
Copy link
Contributor Author

comment:4

ok, j'ai fait les modifications suggerées. Il se trouve qu'elles n'etaient sans doute pas utiles, car le code ne modifiait pas les parametres.

@dcoudert
Copy link
Contributor

dcoudert commented Dec 5, 2021

comment:5

LGTM.

Peut-être pas indispensable en effet pour le moment, mais ça peut éviter des problèmes dans le futur.

@vbraun
Copy link
Member

vbraun commented Jan 31, 2022

Changed branch from u/chapoton/32979 to cee5d62

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

4 participants