Fix several things around root systems #3048
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #3048 +/- ##
==========================================
- Coverage 80.43% 80.42% -0.01%
==========================================
Files 506 506
Lines 70074 70105 +31
==========================================
+ Hits 56365 56385 +20
- Misses 13709 13720 +11
|
|
@felix-roehrich please review the changes here, even though it has already been merged. I can react to your comments in a future PR. |
| end | ||
|
|
||
| function simple_roots(R::RootSystem) | ||
| return [positive_root(R, i) for i in 1:num_simple_roots(R)] |
There was a problem hiding this comment.
| return [positive_root(R, i) for i in 1:num_simple_roots(R)] | |
| return positive_roots(R)[1:rank(R)] |
There was a problem hiding this comment.
The current solution is more efficient than your suggestion. Your suggestion first creates a vector of all positive roots and then copies the first rk many to a new vector. As there are many more positive than simple roots, this is mostly useless work.
There was a problem hiding this comment.
No, the implementation is just an accessor with a type assertion. The suggestion instead prevents creating a new underlying array each time.
There was a problem hiding this comment.
Oh yeah, you are correct.
| word = deepcopy(x.word) | ||
| for _ in 2:n | ||
| for s in Iterators.reverse(x.word) | ||
| _lmul!(x.parent.refl, word, s) | ||
| end | ||
| end | ||
|
|
||
| return WeylGroupElem(x.parent, word) |
There was a problem hiding this comment.
| word = deepcopy(x.word) | |
| for _ in 2:n | |
| for s in Iterators.reverse(x.word) | |
| _lmul!(x.parent.refl, word, s) | |
| end | |
| end | |
| return WeylGroupElem(x.parent, word) | |
| px = deepcopy(x) | |
| for _ in 2:n | |
| for s in Iterators.reverse(x.word) | |
| lmul!(px, Int(s)) # make lmul! more general to make casting unnecessary | |
| end | |
| end | |
| return px |
While writing a jupyter notebook for an exercise class, I found several things that weren't working or just weren't there. This is the first batch of fixes and additions from that.