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

LieAlgebras: Change normal form for WeylGroupElem; cleanup after #4374 #4375

Merged
merged 5 commits into from
Dec 5, 2024

Conversation

lgoettgens
Copy link
Member

@lgoettgens lgoettgens commented Dec 2, 2024

This PR contains steps 2 and 3 of the plan laid out in #4374 (review).

This currently contains the changes from #4374 (the first three commits in the history). Once that one is merged, I'll rebase this here.

Copy link

codecov bot commented Dec 3, 2024

Codecov Report

Attention: Patch coverage is 94.28571% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84.37%. Comparing base (0c5350a) to head (a4942b9).
Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
experimental/LieAlgebras/src/WeylGroup.jl 93.75% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4375      +/-   ##
==========================================
+ Coverage   84.36%   84.37%   +0.01%     
==========================================
  Files         656      656              
  Lines       87196    87189       -7     
==========================================
+ Hits        73559    73562       +3     
+ Misses      13637    13627      -10     
Files with missing lines Coverage Δ
...imental/AlgebraicShifting/src/PartialShiftGraph.jl 87.34% <ø> (ø)
experimental/LieAlgebras/src/Types.jl 96.73% <100.00%> (ø)
experimental/LieAlgebras/src/WeightLattice.jl 87.56% <100.00%> (ø)
experimental/LieAlgebras/test/WeylGroup-test.jl 97.87% <ø> (ø)
experimental/LieAlgebras/src/WeylGroup.jl 90.98% <93.75%> (-0.03%) ⬇️

... and 4 files with indirect coverage changes

@lgoettgens lgoettgens closed this Dec 4, 2024
@lgoettgens lgoettgens reopened this Dec 4, 2024
@lgoettgens
Copy link
Member Author

lgoettgens commented Dec 4, 2024

@antonydellavecchia the CI here fails in your algebraic shifting code.
Let me point out the two changes in this PR here:

  1. Some Weyl group elements print differently (since they use a different internal word)
  2. iterating a Weyl group now yields a different order of elements

both things are just changes to internals, so you should not rely on them. In particular, for 2. it could maybe make sense to change your edge labels from vectors to sets. I cannot judge if that makes sense semantically for the graphs you are constructing, but it would get rid of the indirect dependency on the order in

Ks -> multi_edges(F, phi.(W), Ks, complex_labels),
.

If you agree, I would just adapt the corresponding tests inside this PR, aka change the order of weyl group elements in vectors, if this change comes from a change in order when iterating the weyl group. Alternatively, I could change the == in

@test word.(edge_labels[2, 1]) == word.([s[1], s[1] * s[2], s[1] * s[2] * s[1], s[2] * s[1]])
to an issetequal.

See 39bfd9e (#4375) for my preliminary changes to these tests.

@lgoettgens lgoettgens marked this pull request as ready for review December 4, 2024 12:59
@antonydellavecchia
Copy link
Collaborator

@antonydellavecchia the CI here fails in your algebraic shifting code. Let me point out the two changes in this PR here:

1. Some Weyl group elements print differently (since they use a different internal word)

2. iterating a Weyl group now yields a different order of elements

both things are just changes to internals, so you should not rely on them. In particular, for 2. it could maybe make sense to change your edge labels from vectors to sets. I cannot judge if that makes sense semantically for the graphs you are constructing, but it would get rid of the indirect dependency on the order in

Ks -> multi_edges(F, phi.(W), Ks, complex_labels),

.

If you agree, I would just adapt the corresponding tests inside this PR, aka change the order of weyl group elements in vectors, if this change comes from a change in order when iterating the weyl group. Alternatively, I could change the == in

@test word.(edge_labels[2, 1]) == word.([s[1], s[1] * s[2], s[1] * s[2] * s[1], s[2] * s[1]])

to an issetequal.

See 39bfd9e (#4375) for my preliminary changes to these tests.

@antonydellavecchia the CI here fails in your algebraic shifting code. Let me point out the two changes in this PR here:

1. Some Weyl group elements print differently (since they use a different internal word)

2. iterating a Weyl group now yields a different order of elements

both things are just changes to internals, so you should not rely on them. In particular, for 2. it could maybe make sense to change your edge labels from vectors to sets. I cannot judge if that makes sense semantically for the graphs you are constructing, but it would get rid of the indirect dependency on the order in

Ks -> multi_edges(F, phi.(W), Ks, complex_labels),

.

If you agree, I would just adapt the corresponding tests inside this PR, aka change the order of weyl group elements in vectors, if this change comes from a change in order when iterating the weyl group. Alternatively, I could change the == in

@test word.(edge_labels[2, 1]) == word.([s[1], s[1] * s[2], s[1] * s[2] * s[1], s[2] * s[1]])

to an issetequal.

See 39bfd9e (#4375) for my preliminary changes to these tests.

I think changing to an issetequal might be beneficial for long term. the ordering on the edge label doesn't matter and they could have been sets instead.

@lgoettgens lgoettgens requested a review from fingolfin December 5, 2024 10:57
@fingolfin fingolfin merged commit 3b56941 into oscar-system:master Dec 5, 2024
30 checks passed
@lgoettgens lgoettgens deleted the lg/weyl-normal-form branch December 5, 2024 22:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experimental Only changes experimental parts of the code topic: LieTheory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants