-
Notifications
You must be signed in to change notification settings - Fork 126
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
Integrating JuLie into Oscar #1935
Changes from 41 commits
d551e3c
b1e5acb
73c8aa5
47d100f
40800e9
e546118
8cfd689
434f2e5
46d368f
d04b2b3
d9aa891
6323ad8
72e0499
cb18012
f388280
caa6abe
e7e6270
d283f71
cdf832a
a6375f5
d780ff6
eee0bc5
daceaad
004c27d
8fb8525
949b308
b1f919a
59fbf75
7c24e21
d8ba513
606bf7e
2c4eeea
d956d45
15f5c9b
d60fce9
9eebe66
7771cba
b134e27
d860db8
c85323d
9a5b502
25037aa
df6d179
d7602e1
8347cd2
e584a33
40e9ad9
855bd3c
96dcaa0
b730860
1e6302a
70bc9e3
0747f7d
1117ee3
be5494a
f3779da
be8ae24
71b0fb5
873f33a
7cecf18
9c14de8
76fff7a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
# Partitions | ||
|
||
```@docs | ||
Partition | ||
``` | ||
|
||
## Unrestricted partitions | ||
|
||
```@docs | ||
partitions(::Integer) | ||
num_partitions(::Integer) | ||
ascending_partitions | ||
``` | ||
|
||
## Restricted partitions | ||
|
||
```@docs | ||
partitions(::Integer, ::Integer, ::Integer, ::Integer) | ||
partitions(::Integer, ::Integer) | ||
num_partitions(::Integer, ::Integer) | ||
partitions(::Array{Integer,1}, ::Integer, ::Array{Integer,1}, ::Integer) | ||
ClaudiaHeYun marked this conversation as resolved.
Show resolved
Hide resolved
|
||
``` | ||
|
||
## Operations | ||
|
||
```@docs | ||
conjugate | ||
getindex_safe | ||
``` | ||
|
||
## Relations | ||
|
||
```@docs | ||
dominates | ||
``` | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
# Schur polynomials | ||
|
||
```@docs | ||
schur_polynomial(lambda::Partition{T}, n=sum(lambda)::Int) where T<:Integer | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
# Tableaux | ||
|
||
```@docs | ||
Tableau | ||
``` | ||
|
||
## Operations | ||
|
||
```@docs | ||
hook_length | ||
hook_lengths | ||
shape | ||
weight | ||
reading_word | ||
``` | ||
|
||
## Semistandard tableaux | ||
|
||
```@docs | ||
is_semistandard | ||
semistandard_tableaux | ||
``` | ||
|
||
## Standard tableaux | ||
|
||
```@docs | ||
is_standard | ||
standard_tableaux | ||
num_standard_tableaux | ||
schensted | ||
bump! | ||
``` | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -184,6 +184,53 @@ end | |
|
||
However, as always, rules sometimes should be broken. | ||
|
||
## Optional arguments for parents of return values | ||
|
||
Several objects in Oscar have `parent`s, e.g. polynomials, group elements, ... | ||
ClaudiaHeYun marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Whenever a function creates such objects from an input which does not involve | ||
the output's parent, the user should have the possibility to pass on this | ||
parent as an optional argument. | ||
|
||
Let's see an example. Say, you want to implement the characteristic | ||
polynomial of a matrix. You could do it as follows: | ||
```julia | ||
function characteristic_polynomial(A::MatrixElem) | ||
kk = base_ring(A) | ||
P, x = kk["x"] | ||
AP = change_base_ring(P, A) | ||
return det(AP - x*one(AP)) | ||
end | ||
``` | ||
You can see that the polynomial ring `P`, i.e. the parent of the output, | ||
is newly created in the body of the function. In particular, calling this | ||
function two times on two different matrices `A` and `B` might produce | ||
incompatible polynomials `p = det(A - x*one(A))` and `q = det(B - x*one(B))` | ||
with different parents. Then `p + q` will produce an error. | ||
|
||
To solve this, we should have implemented the function differently: | ||
```julia | ||
function characteristic_polynomial( | ||
A::MatrixElem; | ||
ring::AbstractAlgebra.Ring=(base_ring(A)["t"])[1] | ||
) | ||
AP = change_base_ring(ring, A) | ||
x = first(gens(ring)) | ||
return det(AP - x*one(AP)) | ||
end | ||
|
||
function characteristic_polynomial( | ||
P::AbstractAlgebra.Ring, | ||
A::MatrixElem | ||
) | ||
coefficient_ring(P) === base_ring(A) || error("coefficient rings incompatible") | ||
return characteristic_polynomial(A, ring=P) | ||
end | ||
``` | ||
This allows for two different entry points for the ring `P` of the output: | ||
Once as the first argument of a method of `characteristic_polynomial` with | ||
an extended signature, and second as an optional keyword argument for `ring` in | ||
the original method. This should be the general rule for such implementations | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are editing the OSCAR styleguide here. Either this is the general rule within OSCAR, or it isn't; the "should" makes no sense. Whether this is the rule is debatable, though. Do we really want to force all code to offer both these variants? Why? I.e., why two ways to achieve the same thing? Also, perhaps Indeed, as far as I can tell, the "real" My recommendation: move this addition to the styleguide into a separate PR (it doesn't really belong into this PR anyway). That way, we can discuss it there, and don't hold up this PR here. |
||
within Oscar. | ||
|
||
## Deprecating functions | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
module JuLie | ||
|
||
using ..Oscar | ||
using Markdown | ||
import Nemo:libflint | ||
fingolfin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
include("JuLie/partitions.jl") | ||
include("JuLie/schur_polynomials.jl") | ||
include("JuLie/tableaux.jl") | ||
|
||
end | ||
|
||
using .JuLie | ||
export Partition, partitions, ascending_partitions, dominates, conjugate, getindex_safe, num_partitions, schur_polynomial, Tableau, shape, semistandard_tableaux, is_standard, is_semistandard, standard_tableaux, schensted, hook_length, hook_lengths, num_standard_tableaux, reading_word, weight, bump! | ||
ClaudiaHeYun marked this conversation as resolved.
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.
Meta-question: what is
Integer
here? The Julia typeBase.Integer
?Can one also pass the OSCAR integer type
fmpz
to these functions?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.
Yes, it's
Base.Integer
. Passingfmpz
results in an error. Should we do something about this?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.
It would be great if
IntegerUnion
was accepted, i.e. bothfmpz
(=ZZRingElem
) andBase.Integer
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.
(But this is not a requirement for merging, just something that would be nice to have eventually -- if we don't do it in this PR, someone should open an issue to remind us to do it)
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.
I fixed
partitions
to acceptfmpz
integers. However, there are a number of other functions that take integers as inputs. Should we modify everything so that they all acceptIntegerUnion
?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.
I changed all functions in
partitions.jl
to acceptIntegerUnion
. I didn't do that forschur_polynomials.jl
ortableaux.jl
. Hope this doesn't break anything... 😬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.
That was my problem as well and I didn't really know what to do about it.
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.
Ideally, any place that would accept a BigInt should also accept an
fmpz
/ZZRingElem
.Of course there are places where really only a "small" integer makes sense, in those case staying with
Integer
is fine.