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

refresh the file subword.py #33555

Closed
fchapoton opened this issue Mar 24, 2022 · 24 comments
Closed

refresh the file subword.py #33555

fchapoton opened this issue Mar 24, 2022 · 24 comments

Comments

@fchapoton
Copy link
Contributor

Various details:

  • pep8 cleanup
  • annotations
  • add method __ne__
  • tweaks in smallest_positions
  • typos

CC: @tscrim @slel @kwankyu

Component: combinatorics

Author: Frédéric Chapoton

Branch/Commit: 5d5a72f

Reviewer: Travis Scrimshaw, Samuel Lelièvre

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

@fchapoton fchapoton added this to the sage-9.6 milestone Mar 24, 2022
@fchapoton
Copy link
Contributor Author

Branch: u/chapoton/33555

@fchapoton
Copy link
Contributor Author

Commit: e411b13

@fchapoton
Copy link
Contributor Author

New commits:

e411b13refresh the file combinat/subword.py

@fchapoton

This comment has been minimized.

@fchapoton
Copy link
Contributor Author

comment:3

green bot, so please review

@tscrim
Copy link
Collaborator

tscrim commented Mar 27, 2022

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Mar 27, 2022

comment:4

One extra change would be __repr__() -> _repr_() (so rename() can work). If you don't want to do that, then you can set a positive review.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 27, 2022

Changed commit from e411b13 to c966ef1

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 27, 2022

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

c966ef1using `_repr_` in subword

@fchapoton
Copy link
Contributor Author

comment:7

thanks, ok, here we go.

This file seems to be used nowhere inside sage..

@tscrim
Copy link
Collaborator

tscrim commented Mar 27, 2022

comment:8

Thanks.

Indeed, but it is globally imported in combinat.all. Perhaps we should also take this opportunity to make it a lazy_import?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 27, 2022

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

4081734lazify import of subword

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 27, 2022

Changed commit from c966ef1 to 4081734

@fchapoton
Copy link
Contributor Author

comment:10

good idea. Voilà

@tscrim
Copy link
Collaborator

tscrim commented Mar 27, 2022

comment:11

Thanks. LGTM.

@slel
Copy link
Member

slel commented Mar 27, 2022

comment:12

For multiline imports, how about

  • parentheses instead of line continuation with backslash?
  • sorting by alphabet or theme (unless the order reflects
    dependencies among imports)?

For instance:

-from .combinat import bell_number, catalan_number, euler_number, fibonacci, \
-    lucas_number1, lucas_number2, stirling_number1, stirling_number2, \
-    polygonal_number, CombinatorialObject, CombinatorialClass, \
-    MapCombinatorialClass, \
-    tuples, number_of_tuples, unordered_tuples, number_of_unordered_tuples, \
-    bell_polynomial, fibonacci_sequence, fibonacci_xrange, bernoulli_polynomial
+from .combinat import (
+    CombinatorialClass, CombinatorialObject, MapCombinatorialClass,
+    bell_number, bell_polynomial, bernoulli_polynomial,
+    catalan_number, euler_number,
+    fibonacci, fibonacci_sequence, fibonacci_xrange,
+    lucas_number1, lucas_number2, polygonal_number,
+    stirling_number1, stirling_number2,
+    tuples, number_of_tuples,
+    unordered_tuples, number_of_unordered_tuples
+)

Maybe reword "inheritate from when needed"
to "inherit from it when needed".

Maybe rephrase:

-As repetition can occur in the initial word, the subwords of a given word is
-not a set in general but an enumerated multiset!
+As repetition can occur in the initial word, in general subwords
+of a given word form an enumerated multiset rather than a set!

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 27, 2022

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

5d5a72freviewer's suggested details

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 27, 2022

Changed commit from 4081734 to 5d5a72f

@fchapoton
Copy link
Contributor Author

comment:14

voilà, c'est fait

@slel
Copy link
Member

slel commented Mar 27, 2022

Changed reviewer from Travis Scrimshaw to Travis Scrimshaw, Samuel Lelièvre

@slel
Copy link
Member

slel commented Mar 27, 2022

comment:15

I think "inheritate" should be "inherit" if you can fix that too.

More long import lines could be wrapped but not necessarily here.

If bots green, good to go.

@slel

This comment has been minimized.

@fchapoton
Copy link
Contributor Author

comment:16

setting to positive, thanks

@vbraun
Copy link
Member

vbraun commented Apr 2, 2022

Changed branch from u/chapoton/33555 to 5d5a72f

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