Opened 3 years ago

Closed 3 years ago

#27640 closed enhancement (fixed)

Shi arrangement for other types

Reported by: etzanaki Owned by:
Priority: major Milestone: sage-8.8
Component: combinatorics Keywords: days98
Cc: VivianePons, tscrim Merged in:
Authors: Eleni Tzanaki, Viviane Pons Reviewers: Frédéric Chapoton, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: b176a2a (Commits, GitHub, GitLab) Commit: b176a2a9d6c11eeda227e969aebe02bc28b15f84
Dependencies: Stopgaps:

Status badges

Description (last modified by etzanaki)

m-extended Shi arrangement defined for any finite crystallograpic root system

Change History (16)

comment:1 Changed 3 years ago by etzanaki

  • Cc @… added
  • Component changed from PLEASE CHANGE to combinatorics
  • Description modified (diff)
  • Keywords #days98 added

comment:2 Changed 3 years ago by etzanaki

  • Cc VivianePons added; @… removed

comment:3 Changed 3 years ago by tscrim

  • Cc tscrim added
  • Keywords days98 added; #days98 removed
  • Type changed from PLEASE CHANGE to enhancement

comment:4 Changed 3 years ago by etzanaki

  • Branch set to u/etzanaki/shi_arrangement_for_other_types

comment:5 Changed 3 years ago by etzanaki

  • Authors set to Eleni Tzanaki
  • Commit set to 0924213f7cc85bca0f3ed072605cab1fca817d6d
  • Status changed from new to needs_review

New commits:

0924213Adding cartan type and m-extended Shi arrangments

comment:6 Changed 3 years ago by VivianePons

  • Branch changed from u/etzanaki/shi_arrangement_for_other_types to u/VivianePons/shi_arrangement_for_other_types

comment:7 Changed 3 years ago by tscrim

  • Commit changed from 0924213f7cc85bca0f3ed072605cab1fca817d6d to 411f5e1bcb875fe8ee5907ee4312220ac1f4cd35

Some PEP8 and other misc small comments:

-    def Shi(self, data, K=QQ, names=None, m = 1):
+    def Shi(self, data, K=QQ, names=None, m=1):
         The characteristic polynomial is pre-computed using the results of
-         [Ath1996]_ .
+        [Ath1996]_.
         if data in NN:
-            cartan_type = CartanType(["A",data -1])
+            cartan_type = CartanType(['A', data-1])
         for a in PR:
             for const in range(-m+1,m+1):
-                hyperplanes.append(sum(a[j]*x[j] for j in range(d
-                ))-const)
+                hyperplanes.append(sum(a[j]*x[j] for j in range(d))-const)

(the 80 char/line is much more of a guideline and you should never sacrifice readability for it IMO).

-        charpoly = (x**(d-n))*(x-m*h)**n
+        charpoly = x**(d-n) * (x - m*h)**n

It would also be nice to remove those added blanklines at the end of the file.


New commits:

411f5e1getting rid of trailing white spaces and some mysterious "non ascii character"

comment:8 Changed 3 years ago by git

  • Commit changed from 411f5e1bcb875fe8ee5907ee4312220ac1f4cd35 to f1bd24e876b88fb3fcc6dda81d5d4a33c0795099

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

f1bd24ePEP8 small changes and removing blanklines

comment:9 Changed 3 years ago by VivianePons

It looks like the bot is happy. I have fixed the small things pointed by Travis.

Eleni wrote the initial code and I helped with the design and details. The code extends the previous functionalities of the function. The previous usage is still valid and works exactly the same (we haven't changed those tests) and now we have more options (with Cartan types and m parameter), all documented. Eleni checked the math quite carefully.

So Travis, I let you have a look and hopefully, we can have a positive review soon.

comment:10 Changed 3 years ago by chapoton

  • avoid using unicode "–" in the references, just use - (minus)
  • in the doctests, write h = hyperplane, namely add space around =
  • do not repeat the "EXAMPLES::"

otherwise, looks good

comment:11 Changed 3 years ago by chapoton

and this line is useless, as told by the patchbot:

R = RootSystem(cartan_type)

comment:12 Changed 3 years ago by git

  • Commit changed from f1bd24e876b88fb3fcc6dda81d5d4a33c0795099 to b176a2a9d6c11eeda227e969aebe02bc28b15f84

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

b176a2aSmall fixes following comments

comment:13 Changed 3 years ago by VivianePons

All done!

comment:14 Changed 3 years ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

ok, thx

comment:15 Changed 3 years ago by tscrim

  • Authors changed from Eleni Tzanaki to Eleni Tzanaki, Viviane Pons
  • Reviewers changed from Frédéric Chapoton to Frédéric Chapoton, Travis Scrimshaw

Viviane, I added you as an author based on comment:9. If you feel you closer to a reviewer, feel free to move yourself over.

comment:16 Changed 3 years ago by vbraun

  • Branch changed from u/VivianePons/shi_arrangement_for_other_types to b176a2a9d6c11eeda227e969aebe02bc28b15f84
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.