Integrating JuLie into Oscar #1935

ClaudiaHeYun · 2023-02-14T11:03:38Z

Three files from JuLie are included: partitions, schur_polynomials, tableaux. Their tests and documentations are included too. We stopped importing partitions from Hecke. Right now there is a problem with YoungTableau, which uses partitions from AbstractAlgebra. For example,

p = Partition([4,3,2])
yt = YoungTableau(p)

will create an error. This can be fixed by specifying p = AbstractAlgebra.general.Partition([4,3,2]). Eventually it would be nice to merge Tableau and YoungTableau.

Add testset wrappers.

Some polishing of docstrings

…ie_yun

Add section on optional arguments for parents of return values.

…ie_yun

Add first file for documentation.

…into julie_yun

Adjust indentation.

fingolfin

A couple more comments.

I will shortly push several commits to this PR to address some of my own remarks. Please make sure to pull before (force) pushing anything to this PR / branch, to avoid loosing my changes

fingolfin · 2023-03-02T08:30:31Z

experimental/JuLie/tableaux.jl

+
+@Markdown.doc """
+    schensted(sigma::Vector{Integer})
+    schensted(sigma::Perm{T})


Note that Perm{T} is from AbstractAlgebra, but we don't use it in OSCAR -- we have our own permutation type here, PermGroupElem

That permutation type definitely should be supported here and in other places taking permutations. (To be clear: I am OK with supporting Perm{T}, too, at least for now)

I find it debatable whether a permutation really needs to be an element of a group. I feel there should be both (a permutation as an element of a group and a permutation just by itself) as separate types.

But again, such discussions should not stop any of the real work.

Sure. And note that I did not ask for Perm{T} support to be dropped, but rather for support for PermGroupElem to be added, as that is the kind of permutation most OSCAR users will encounter.

But this is not a blocker for this PR.

experimental/JuLie/tableaux.jl

experimental/JuLie.jl

fingolfin · 2023-03-09T23:10:15Z

docs/src/DeveloperDocumentation/styleguide.md

+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 


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 ring is not always the best name for the argument, and from the text it sounds this is mandatory.

Indeed, as far as I can tell, the "real" charpoly and minpoly do not support the keyword argument variant.

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.

experimental/Experimental.jl

experimental/JuLie/partitions.jl

fingolfin · 2023-03-09T23:17:54Z

experimental/JuLie/partitions.jl

+
+
+@Markdown.doc """
+    conjugate(lambda::Partition{T}) where T<:IntegerUnion


The following is a purely informal remark, I don't expect you to do anything about it:

Note that in Julia (and OSCAR) this is normally called conj. However, I've added a suggestion to #1826 that we should allow conjugate as a synonym for conj.

experimental/JuLie/schur_polynomials.jl

Long lines result in near useless diffs when editing them

fingolfin · 2023-03-10T00:02:08Z

IntegerUnion is the way to go. You always want to allow Integers since people type partitions(4) and not partitions(ZZRingElem(4)) or ZZ(4) However, programmantic, most functions in Oscar return ZZRingElem = fmpz, so they should be valid input
…

Ok. But if an fmpz becomes a loop variable (e.g., for i=1 to n when n::fmpz) this may be not so good.

If changing Integer to IntegerUnion allows a loop variable to become an ZZRingElem, then the code already allowed it to become a `BigInt, which is equally bad. There are various ways to address this, but of course it depends on the code.

E.g. if the code really only makes sense for small inputs (e.g. factorial(n) with n >= 2^64 is not something we can actually compute), then one can write code like this:

my_factorial(n::T) where T <: IntegerUnion = my_factorial(T, Int(n))
function my_factorial(::Type{T}, n::Int) where T <: IntegerUnion
  @req n >= 0 "n must be non-negative"
  x = T(1)
  for i in 1:n
    x = mul!(x, x, i)
  end
  return x
end

Anyways, such stuff shouldn't stop merging, can be adapted later.

Agreed

fingolfin · 2023-03-10T00:25:02Z

experimental/JuLie/partitions.jl

+julia> @btime partitions(Int8(90));
+3.376 s (56634200 allocations: 6.24 GiB)
+
+julia> @btime ascending_partitions(Int8(90),alg="ks");
+3.395 s (56634200 allocations: 6.24 GiB)
+
+julia> @btime ascending_partitions(Int8(90),alg="m");
+3.451 s (56634200 allocations: 6.24 GiB)


Interesting! With which Julia version, OS and hardware did you obtain these benchmark results?

On my M1 MacBook with Julia 1.9.0-rc1, ascending_partitions is actually twice as fast as partitions:

julia> @btime partitions(Int8(90)); 8.959 s (56634195 allocations: 4.53 GiB) julia> @btime ascending_partitions(Int8(90),alg="ks"); 4.546 s (56634195 allocations: 4.53 GiB) julia> @btime ascending_partitions(Int8(90),alg="m"); 4.616 s (56634195 allocations: 4.53 GiB)

Oh! With Julia 1.8.5, it is much faster and looks more like your numbers:

julia> @btime partitions(Int8(90)); 2.123 s (56634195 allocations: 4.53 GiB) julia> @btime ascending_partitions(Int8(90),alg="ks"); 2.478 s (56634195 allocations: 4.53 GiB) julia> @btime ascending_partitions(Int8(90),alg="m"); 2.432 s (56634195 allocations: 4.53 GiB)

So there seems to be a major performance regression in Julia 1.9 :-(. Wonder if we can pinpoint it...

This is weird. I believe my old benchmarks were done even with Julia 1.7.

fingolfin · 2023-03-10T00:27:26Z

experimental/JuLie/partitions.jl

+sequence (which is our convention), one can also encode it as an *ascending*
+sequence. In the papers Kelleher & O'Sullivan (2014) and Merca (2012) it is
+said that generating the list of all ascending partitions is more efficient
+than generating descending ones. To test this, I have implemented the


This docstring uses "I" quite a bit: "I have", "I am", .... For an OSCAR user it won't be clear to whom that "I" refers. So we should rewrite it, either using "we"; or using "U. Thiel"

@ulthiel any preferences?

Well, this was my personal documentation, so of course I used "I". I don't know. It's maybe also not so important.

Noticing that the ascending_partitions stuff is in the pull request as well: I later changed this to ascending_compositions (which was part of my composition generator) because it's technically not a partition by definition. Since the composition stuff is currently not in the pull request (and not in JuLie) I'm unsure what should happen with this functionality. It should definitely be there eventually.

Of course it made perfect sense to use "I" while this was in JuLie, and I am not criticizing this. I am merely pointing out that it doesn't make sense anymore in the context of Oscar, because most readers won't know whom "I" refers to. Moreover, it differs from the style we used elsewhere. Hence it should be changed. I can easily change it now, of course, but as a courtesy I thought I'd ask you for your preferences...?

Regarding your second reply: Note that this is the docstring for ascending_partitions we are discussing here. So you are saying you renamed it later anyway, and moved it to some other module... So should we just delete it from this PR for now and it'll be added back later?

fingolfin · 2023-03-10T00:28:28Z

experimental/JuLie/partitions.jl

+
+
+@Markdown.doc """
+    partitions(m::IntegerUnion, n::IntegerUnion, l1::IntegerUnion, l2::IntegerUnion; z=0)


The type for z here should match the declaration below

fingolfin · 2023-03-10T00:29:55Z

experimental/JuLie/partitions.jl

+
+The algorithm used is "parta" in [RJ76](@cite), de-gotoed from old ALGOL code by E. Thiel!
+"""
+function partitions(m::IntegerUnion, n::IntegerUnion, l1::IntegerUnion, l2::IntegerUnion; z::IntegerUnion=0)


If z only allows for two choices, there is no point in allowing it to be IntegerUnion, it can go back to being Int.

However, why isn't this a Bool with a speaking name? E.g.

Suggested change

function partitions(m::IntegerUnion, n::IntegerUnion, l1::IntegerUnion, l2::IntegerUnion; z::IntegerUnion=0)

function partitions(m::IntegerUnion, n::IntegerUnion, l1::IntegerUnion, l2::IntegerUnion; only_distinct_parts::Bool = false)

fingolfin · 2023-03-10T00:32:38Z

experimental/JuLie/partitions.jl

+julia> @btime partitions(Int8(90));
+3.376 s (56634200 allocations: 6.24 GiB)
+
+julia> @btime ascending_partitions(Int8(90),alg="ks");
+3.395 s (56634200 allocations: 6.24 GiB)
+
+julia> @btime ascending_partitions(Int8(90),alg="m");
+3.451 s (56634200 allocations: 6.24 GiB)


Oh! With Julia 1.8.5, it is much faster and looks more like your numbers:

julia> @btime partitions(Int8(90)); 2.123 s (56634195 allocations: 4.53 GiB) julia> @btime ascending_partitions(Int8(90),alg="ks"); 2.478 s (56634195 allocations: 4.53 GiB) julia> @btime ascending_partitions(Int8(90),alg="m"); 2.432 s (56634195 allocations: 4.53 GiB)

So there seems to be a major performance regression in Julia 1.9 :-(. Wonder if we can pinpoint it...

micjoswig · 2023-03-11T08:55:37Z

It seems that the test (1.6, x64, ubuntu-latest) was killed only because some test cases took too long?

If you agree, merge this PR now. And create a new issue to reduce the time for testing. At the end of the test output the biggest offenders are listed. Add them explicitly to the new issue.

@fingolfin @benlorenz

fingolfin · 2023-03-12T15:55:13Z

The tests of this PR are not slow, I already verified this.

I already spent some effort last week to clean up this PR and fix a bunch of issues with it. It's unfortunate that nobody else really had timte to carefully review this code.

The remaining blocker for me is the part where it changes the developer documentation. I will remove this now from the PR and adjust some more things. If someone wants to re-submit it in a separate PR, they are welcome, we can then discuss it there.

ClaudiaHeYun and others added 30 commits February 2, 2023 16:20

first julie test

d551e3c

Add testset wrappers.

b1e5acb

Merge pull request #1 from HechtiDerLachs/julie_yun

73c8aa5

Add testset wrappers.

excluding Partition from Hecke; including partitions from JuLie

47d100f

passed partitions test

40800e9

Turn lambda and mu to non-greek letters.

e546118

Add Markdown statements.

8cfd689

Export references.

434f2e5

Merge pull request #2 from HechtiDerLachs/julie_yun

46d368f

Some polishing of docstrings

add schur polynomials and test file

d04b2b3

Merge branch 'julie_yun' of github.com:ClaudiaHeYun/Oscar.jl into jul…

d9aa891

…ie_yun

fix bug in schur_polynomials.jl; re-structure module

6323ad8

Add section on optional arguments for parents of return values.

72e0499

Merge pull request #3 from HechtiDerLachs/julie_yun

cb18012

Add section on optional arguments for parents of return values.

add tableaux and tests; tests passed

f388280

Merge branch 'julie_yun' of github.com:ClaudiaHeYun/Oscar.jl into jul…

caa6abe

…ie_yun

Add first file for documentation.

e7e6270

Rename file.

d283f71

Clarify origins and modifications.

cdf832a

add doc string; export more functions from partitions.jl

a6375f5

Add further documentation for JuLie.

d780ff6

Merge pull request #4 from HechtiDerLachs/julie_yun

eee0bc5

Add first file for documentation.

Adjust indentation.

daceaad

change doc strings

004c27d

Merge branch 'julie_yun' of https://github.com/ClaudiaHeYun/Oscar.jl …

8fb8525

…into julie_yun

Merge pull request #5 from HechtiDerLachs/julie_yun

949b308

Adjust indentation.

Overhaul docstrings.

b1f919a

Adjust indentation.

59fbf75

Adjust references.

7c24e21

Minor fix in the Markdown macro.

d8ba513

fingolfin added 4 commits March 9, 2023 23:51

Remove .DS_Store files

855bd3c

Remove extra whitespace

96dcaa0

Access libflint consistently

b730860

Follow file name conventions

1e6302a

fingolfin reviewed Mar 9, 2023

View reviewed changes

experimental/JuLie/schur_polynomials.jl Outdated Show resolved Hide resolved

fingolfin added 4 commits March 10, 2023 00:35

Change examples to use jldoctest, fix them

70bc9e3

Limit exports to one per line, sort alphabetically

0747f7d

Tweak docstrings, avoid overly long lines

1117ee3

Long lines result in near useless diffs when editing them

Fix misplaced argument type specifications

be5494a

fingolfin reviewed Mar 10, 2023

View reviewed changes

fingolfin closed this Mar 10, 2023

fingolfin reopened this Mar 10, 2023

fingolfin added 3 commits March 11, 2023 01:43

Fix docs

f3779da

Undo random whitespace insertion

be8ae24

Typo

71b0fb5

fingolfin added 4 commits March 12, 2023 17:15

Revise partitions with 4 integer arguments

873f33a

Revert dev docs changes

7cecf18

Merge branch 'master' into julie_yun

9c14de8

Use @Req

76fff7a

fingolfin enabled auto-merge (squash) March 12, 2023 18:27

fingolfin approved these changes Mar 12, 2023

View reviewed changes

fingolfin merged commit b36cc89 into oscar-system:master Mar 12, 2023

HechtiDerLachs mentioned this pull request Mar 19, 2023

Style guide on optional arguments for parents of return values #2084

Merged

joschmitt mentioned this pull request Jan 9, 2024

Promote JuLie to src #3159

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrating JuLie into Oscar #1935

Integrating JuLie into Oscar #1935

ClaudiaHeYun commented Feb 14, 2023

fingolfin left a comment

fingolfin Mar 2, 2023

ulthiel Mar 10, 2023

fingolfin Mar 11, 2023

fingolfin Mar 9, 2023

fingolfin Mar 9, 2023

fingolfin commented Mar 10, 2023

fingolfin Mar 10, 2023

fingolfin Mar 10, 2023

ulthiel Mar 10, 2023

fingolfin Mar 10, 2023

ulthiel Mar 10, 2023

ulthiel Mar 10, 2023

fingolfin Mar 12, 2023

fingolfin Mar 10, 2023

fingolfin Mar 10, 2023

fingolfin Mar 10, 2023

micjoswig commented Mar 11, 2023 •

edited

Loading

fingolfin commented Mar 12, 2023



		@Markdown.doc """
		conjugate(lambda::Partition{T}) where T<:IntegerUnion



		@Markdown.doc """
		partitions(m::IntegerUnion, n::IntegerUnion, l1::IntegerUnion, l2::IntegerUnion; z=0)

	function partitions(m::IntegerUnion, n::IntegerUnion, l1::IntegerUnion, l2::IntegerUnion; z::IntegerUnion=0)
	function partitions(m::IntegerUnion, n::IntegerUnion, l1::IntegerUnion, l2::IntegerUnion; only_distinct_parts::Bool = false)

Integrating JuLie into Oscar #1935

Integrating JuLie into Oscar #1935

Conversation

ClaudiaHeYun commented Feb 14, 2023

fingolfin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin commented Mar 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micjoswig commented Mar 11, 2023 • edited Loading

fingolfin commented Mar 12, 2023

micjoswig commented Mar 11, 2023 •

edited

Loading