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

Random subquotient module elements and homomorphisms #3122

Merged
merged 4 commits into from
Dec 20, 2023

Conversation

jankoboehm
Copy link
Contributor

Random homogeneous elements of modules, in particular random homomorphisms. Limited to finite base rings, for the moment. What is here already does what we need currently for certain constructions in algebraic geometry. We will expand on it further.

Comment on lines 2892 to 2903
function all_partitions_in(n, k)
if k == 0
return n == 0 ? [Int[]] : []
end
partitions = Vector{Int}[]
for i in 0:n
for p in all_partitions_in(n - i, k - 1)
push!(partitions, [i; p])
end
end
return partitions
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not live here, but in src/Combinatorics as it can be applied elsewhere. One day, it can then be replaced by the implementation in experimental/JuLie/src/partitions.jl.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I don't mind to shift it and make it available. We will also expand on it. As far as I understood JuLi does partitiions in the sorted sense. This here is what is usually called compositions. So I renamed it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for moving!
However, I think that this should either include a docstring as well, or shouldn't get exported (or maybe even both for now).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, changing this later to ZZRingElem instead of Int would be an unnecessary breaking change, so I would rather keep this internal until an interface has been settled. (With talking to people interested in combinatorics like Uli Thiel)

Copy link

codecov bot commented Dec 20, 2023

Codecov Report

Merging #3122 (b6c401e) into master (d8cea53) will increase coverage by 0.00%.
The diff coverage is 95.55%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3122   +/-   ##
=======================================
  Coverage   80.61%   80.61%           
=======================================
  Files         527      528    +1     
  Lines       70721    70766   +45     
=======================================
+ Hits        57009    57051   +42     
- Misses      13712    13715    +3     
Files Coverage Δ
src/Combinatorics/Compositions.jl 100.00% <100.00%> (ø)
src/Oscar.jl 61.03% <ø> (ø)
src/Modules/ModulesGraded.jl 81.78% <91.30%> (+0.23%) ⬆️

... and 1 file with indirect coverage changes


function weak_compositions(n, k)
if k == 0
return n == 0 ? [Int[]] : []
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return n == 0 ? [Int[]] : []
return n == 0 ? [Int[]] : Vector{Int}[]


function compositions(n, k)
if k == 0
return n == 0 ? [Int[]] : []
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return n == 0 ? [Int[]] : []
return n == 0 ? [Int[]] : Vector{Int}[]

@wdecker wdecker merged commit c3d8887 into oscar-system:master Dec 20, 2023
14 of 19 checks passed
@thofma
Copy link
Collaborator

thofma commented Dec 20, 2023

Would have been better to not export this. Not sure this is compatible with the rest of the combinatorics.

@thofma
Copy link
Collaborator

thofma commented Dec 20, 2023

It also has no docstring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants