Add isomorphism(PermGroup, ::WeylGroup) for missing irreducible types#4478
Conversation
|
This is a continuation of #4264, I guess. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4478 +/- ##
==========================================
+ Coverage 84.36% 84.40% +0.03%
==========================================
Files 663 672 +9
Lines 87805 88708 +903
==========================================
+ Hits 74077 74873 +796
- Misses 13728 13835 +107
|
lgoettgens
left a comment
There was a problem hiding this comment.
Thanks! I have a few comments, mostly documentation-related. Once they are resolved, I would be happy to merge this PR. If you prefer to wait until reducible types are working, this would be fine for me as well.
Co-authored-by: Lars Göttgens <lars.goettgens@gmail.com>
Co-authored-by: Lars Göttgens <lars.goettgens@gmail.com>
Co-authored-by: Lars Göttgens <lars.goettgens@gmail.com>
Co-authored-by: Lars Göttgens <lars.goettgens@gmail.com>
|
Thanks for your comments! I resolved everything. I plan to look into the reducible types soonish, but I see no harm in merging already. |
lgoettgens
left a comment
There was a problem hiding this comment.
Thanks for the adaptions! Could you please have a look into the two failing CI jobs? Both should have some instructions in the failure log. Once that's done, this is good to go from my POV.
Co-authored-by: Lars Göttgens <lars.goettgens@gmail.com>
Co-authored-by: Lars Göttgens <lars.goettgens@gmail.com>
Sure, will do that next week. |
|
I applied JuliaFormatter and bibtool and fixed the bib entry (changed "article" to "Article"). Now it's all fine (except for the nightly test). |
| end | ||
| else | ||
| error("Not implemented (yet)") | ||
| Sym = symmetric_group(n + 1) |
There was a problem hiding this comment.
as a general rule of thumb, use lowercase first letters for variables, and reserver Upper case for types. Main except for me is for single letter variables; but even there I'd recommend sym or S here.
But this is not a blocker for this PR, just a general remark for future reference :-)
There was a problem hiding this comment.
Thanks, I will keep that in mind :)
| end | ||
|
|
||
| iso = function (w::WeylGroupElem) | ||
| reduce(*, (gen(G, Int(i)) for i in word(w)); init=one(G)) |
There was a problem hiding this comment.
I would suggest that we instead install a variant of this line as a method for map_word on WeylGroupElem, and then call map_word here.
Of course this can wait for a future PR!
There was a problem hiding this comment.
Sounds good. This would, in particular, remove the potential error source of not converting "i in word(w)" from UInt to Int.
isomorphism(PermGroup, ::WeylGroup) for missing irreducible typesisomorphism(PermGroup, ::WeylGroup) for missing irreducible types
Adds support for all missing irreducible types (B, C, D, E, F, G) in
isomorphism(PermGroup, ::WeylGroup)and also for non-canonical orderings. Reducible types are still missing.@lgoettgens I don't know whether this should be merged already, but I think the code is mature enough for someone else to have a look.