-
Notifications
You must be signed in to change notification settings - Fork 119
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
Experimental: Complex reflection groups #3342
base: master
Are you sure you want to change the base?
Experimental: Complex reflection groups #3342
Conversation
experimental/ComplexReflectionGroups/src/ComplexReflectionGroupType.jl
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks that's a great contribution!
A few quick comments
experimental/ComplexReflectionGroups/src/ComplexReflectionGroupType.jl
Outdated
Show resolved
Hide resolved
experimental/ComplexReflectionGroups/src/ComplexReflectionGroupType.jl
Outdated
Show resolved
Hide resolved
experimental/ComplexReflectionGroups/src/ComplexReflectionGroupType.jl
Outdated
Show resolved
Hide resolved
experimental/ComplexReflectionGroups/src/ComplexReflectionGroupType.jl
Outdated
Show resolved
Hide resolved
experimental/ComplexReflectionGroups/src/ComplexReflectionGroupType.jl
Outdated
Show resolved
Hide resolved
experimental/ComplexReflectionGroups/src/complex_reflection_group.jl
Outdated
Show resolved
Hide resolved
experimental/ComplexReflectionGroups/src/complex_reflection_group.jl
Outdated
Show resolved
Hide resolved
Could you click "resolve conversation" on all comments above once you addressed them? This makes it way easier to keep an overview here. |
Yes, once addressed... ;-) |
If you change the Hecke version in the file |
Excellent. I will when I'm done (ETA next week). |
Done in efbb6e5. |
Can someone (@joschmitt @lgoettgens) help me with the failing tests? This seems to be a problem with experimental/LieAlgebras and the function coroot, which I have implemented (with a different signature) as well. The error is "MethodError: no method matching coroot(::RootSystem, ::Int64)" |
Adding
in |
Thanks, let's see if it works. |
The "long tests" seem to fail because you export |
Ok, checks passed finally. I will need to document the functions, add further documentation, and address the stuff from the top of this conversation. Then it should be OK for a first set of features for complex reflection groups (this can never be finished...). |
experimental/ComplexReflectionGroups/src/complex_reflection_group_LT.jl
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some random comments and nitpickts
experimental/ComplexReflectionGroups/src/complex_reflection_group.jl
Outdated
Show resolved
Hide resolved
experimental/ComplexReflectionGroups/docs/src/complex_reflection_groups.md
Outdated
Show resolved
Hide resolved
########################################################################################### | ||
# Hermitian scalar product | ||
########################################################################################### | ||
function scalar_product(v::AbstractAlgebra.Generic.FreeModuleElem{T}, w::AbstractAlgebra.Generic.FreeModuleElem{T}) where T <: QQAlgFieldElem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, this shouldn't be needed as there already is dot
for a dot-product. But unfortunately, that does not work for non-real fields currently (see Nemocas/Nemo.jl#1694), so this might only be possible to change in the later future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, and for exactly the reason you say: I need to involve complex conjugation on the field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First off, to avoid misunderstandings, let me emphasize that I am OK with merging this as it is for now.
But I still would like to clear this up on the long run. Perhaps it is already clear for @thofma and @fieker but I am not familiar with the background, so I am asking / pointing out the following:
First off, dot
as defined in Julia, does involve complex conjugation:
Compute the dot product between two vectors. For complex vectors, the first vector is conjugated.
However, it doesn't work on AbstractAlgebra.Generic.FreeModuleElem
. For starters iteration over these is not implemented. Nor is conj
implemented for most of our types. Which makes me wonder: why not? We could just add
Base.conj(x::ZZRingElem) = x
Base.conj(x::ZZPolyRing) = x
Base.conj(x::ZZMPolyRing) = x
Base.conj(x::QQFieldElem) = x
Base.conj(x::QQPolyRing) = x
Base.conj(x::QQMPolyRing) = x
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dot
implies conjugation on the first vector, where as @ulthiel wants it on the second vector. We use the same convention as @ulthiel for inner_product
.
I think adding conj
for ring elements, which are contained in the complex numbers is fine. Although I am not sure I want to extend it automatically to polynomials.
experimental/ComplexReflectionGroups/docs/src/complex_reflection_groups.md
Outdated
Show resolved
Hide resolved
experimental/ComplexReflectionGroups/docs/src/complex_reflection_groups.md
Outdated
Show resolved
Hide resolved
experimental/ComplexReflectionGroups/src/complex_reflection_group_CHEVIE.jl
Outdated
Show resolved
Hide resolved
experimental/ComplexReflectionGroups/src/complex_reflection_group.jl
Outdated
Show resolved
Hide resolved
experimental/ComplexReflectionGroups/src/symplectic_reflection_group.jl
Outdated
Show resolved
Hide resolved
experimental/ComplexReflectionGroups/src/symplectic_reflection_group.jl
Outdated
Show resolved
Hide resolved
Okay thanks all. I'll need a bit to address everything... |
What is this failing invalidations? |
If the difference is less than 5 (see e.g. https://github.com/oscar-system/Oscar.jl/actions/runs/8202710398 for the current report), this can be ignored and will usually go away when restarting enough times. |
experimental/ComplexReflectionGroups/docs/src/complex_reflection_groups.md
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3342 +/- ##
==========================================
- Coverage 84.00% 83.86% -0.14%
==========================================
Files 592 603 +11
Lines 81624 83480 +1856
==========================================
+ Hits 68566 70009 +1443
- Misses 13058 13471 +413
|
This is still marked as "draft". @ulthiel what is missing from your POV before this can be merged? I worry a bit about drift happening to this PR (already there is a conflict, though it should be trivial to resolve). I would recommend to merge this rather earlier than later; since it is in experimental, it would still be fine to perform major rewrites later. But once merged at least we can take care of adjusting the code added here as needed to changes in AA/Nemo/Hecke/Julia, and can ensure its tests keep passing, etc. That said, in the end you decided of course. |
Will continue now. |
This PR now contains a lot of other changes not related to complex reflection groups. Could you try to rebase this onto a current master? |
I do not know what this means: 1) changes not related to complex reflection groups? which? 2) rebase? what do I need to do? |
The commit 6d76ba2 contains other changes.
And then follow the instructions in the console to resolve conflicts given by typing |
…tead of returning false
…he property that failts if input is not a complex reflection
6d76ba2
to
d8a1e95
Compare
Ok, is it better now? I have no idea what I just did. I had to resolve conflicts in Experimental.jl and oscar_references.bib several times. My tests passed and I could build the documentation without errors. |
What I still want to do before a merge is the following:
|
This is work in progress implementing complex reflection groups. Contributions welcome.