Skip to content

Add transformations for polyhedral fans and complexes#4444

Merged
benlorenz merged 32 commits intomasterfrom
yr/PolyhedralOperators
Oct 10, 2025
Merged

Add transformations for polyhedral fans and complexes#4444
benlorenz merged 32 commits intomasterfrom
yr/PolyhedralOperators

Conversation

@YueRen
Copy link
Copy Markdown
Member

@YueRen YueRen commented Jan 10, 2025

This draft pull request comes from the Leipzig Workshop aims to add the following new operations:

  • Scalar * PolyhedralFan and PolyhedralFan * Scalar
  • Scalar * PolyhedralComplex and PolyhedralFan * Scalar
  • -PolyhedralFan
  • -PolyhedralComplex
  • Vector + PolyhedralFan and PolyhedralFan + Vector
  • Vector + PolyhedralComplex and PolyhedralComplex + Vector

Questions:

  1. Do we want these functions? My collaborators and I need it for various reasons:
  • negation is useful to construct the negated Bergman fan of a matroid (arises for example here)
  • translation is useful to see how the initial ideals of the tropical intersection points vary (relevant for example for this)

(If yes, I will add the relevant operators for Cone, the operators for Polyhedron already exist)

  1. Is the implementation okay (in particular, I wasn't 100% sure where to put the functions)?

@lgoettgens lgoettgens added the topic: polyhedral geometry Issue concerns polyhedral geometry code label Jan 10, 2025
@YueRen YueRen marked this pull request as ready for review January 14, 2025 08:19
@YueRen YueRen marked this pull request as draft January 14, 2025 08:23
@YueRen YueRen force-pushed the yr/PolyhedralOperators branch from 8617554 to 2a70b17 Compare January 14, 2025 08:30
@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 14, 2025

Codecov Report

❌ Patch coverage is 85.71429% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.99%. Comparing base (207c319) to head (66207cd).
⚠️ Report is 41 commits behind head on master.

Files with missing lines Patch % Lines
...ometry/PolyhedralComplex/standard_constructions.jl 88.88% 4 Missing ⚠️
...alGeometry/PolyhedralFan/standard_constructions.jl 76.92% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4444      +/-   ##
==========================================
- Coverage   84.19%   83.99%   -0.20%     
==========================================
  Files         715      721       +6     
  Lines       97732    98528     +796     
==========================================
+ Hits        82283    82760     +477     
- Misses      15449    15768     +319     
Files with missing lines Coverage Δ
src/PolyhedralGeometry/helpers.jl 83.07% <ø> (-0.53%) ⬇️
src/PolyhedralGeometry/iterators.jl 92.47% <ø> (ø)
...alGeometry/PolyhedralFan/standard_constructions.jl 97.91% <76.92%> (-2.09%) ⬇️
...ometry/PolyhedralComplex/standard_constructions.jl 91.30% <88.88%> (-8.70%) ⬇️

... and 62 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@YueRen YueRen force-pushed the yr/PolyhedralOperators branch from 2a70b17 to 0e34c39 Compare January 14, 2025 14:13
Comment thread src/PolyhedralGeometry/PolyhedralComplex/standard_constructions.jl Outdated
Comment thread src/PolyhedralGeometry/PolyhedralComplex/standard_constructions.jl Outdated
Comment thread src/PolyhedralGeometry/PolyhedralFan/standard_constructions.jl Outdated
@benlorenz
Copy link
Copy Markdown
Member

benlorenz commented Jan 14, 2025

I think it does make sense to have such operations but they should not be limited to rationals and compatible types but probably use scalar_types_extended and then try to coerce the values with coefficient_field(P)(x) (for rational fans / complexes this would give QQ but it would also work for number fields and other types).

You can try with normal_fan(dodecahedron()) as a number field example and normal_fan(n_gon(7)) as a QQBar example.

Edit: And the first argument for the new object should always be the coefficient field of the old object, e.g. polyhedral_fan(coefficient_field(f), ...).

@YueRen YueRen force-pushed the yr/PolyhedralOperators branch from 81c6bc0 to 1f0294c Compare January 14, 2025 15:37
@YueRen
Copy link
Copy Markdown
Member Author

YueRen commented Jan 14, 2025

@benlorenz I tried changing the code as you suggested, but I cannot get it to work. Can you check why the the following addition is undefined?

julia> Sigma = normal_fan(cube(2))
Polyhedral fan in ambient dimension 2

julia> Sigma + [1,1]
ERROR: MethodError: no method matching +(::PolyhedralFan{QQFieldElem}, ::Vector{Int64})
The function `+` exists, but no method is defined for this combination of argument types.

Closest candidates are:
  +(::Any, ::Any, ::Any, ::Any...)
   @ Base operators.jl:596
  +(::Oscar.StraightLinePrograms.Lazy, ::Any)
   @ Oscar ~/.julia/dev/Oscar/src/StraightLinePrograms/lazy.jl:104
  +(::Any, ::Oscar.StraightLinePrograms.Lazy)
   @ Oscar ~/.julia/dev/Oscar/src/StraightLinePrograms/lazy.jl:105
  ...

Stacktrace:
 [1] top-level scope
   @ REPL[4]:1

I was hoping for it to call the following added line in src/PolyhedralGeometry/PolyhedralComplex/standard_constructions.jl:

+(Sigma::PolyhedralFan, v::Vector{<:scalar_types_extended}) = polyhedral_complex(Sigma)+v

@lgoettgens
Copy link
Copy Markdown
Member

julia> Oscar.scalar_types_extended
Union{Float64, FieldElem, ZZRingElem}

it seems that Int is not part of the type union. Let's hope that @benlorenz has an easy solution for this

@YueRen
Copy link
Copy Markdown
Member Author

YueRen commented Jan 15, 2025

In that case, the simplest solution would probably be added all the missing types to scalar_types_extended, provided that is what the type was designed for in the first place.

@benlorenz
Copy link
Copy Markdown
Member

benlorenz commented Jan 15, 2025

I had a look at other uses of that union and it should be fine to change it to Union{scalar_types,IntegerUnion} to allow ZZ and Integer.

Edit: it looks like this might need some extra changes for the ray- and pointvectors ...

@YueRen YueRen force-pushed the yr/PolyhedralOperators branch 2 times, most recently from fc9a919 to 8dbe909 Compare January 24, 2025 09:16
@benlorenz
Copy link
Copy Markdown
Member

from #4445 (comment)

@benlorenz I've changed scalar_types_extended:

-const scalar_types_extended = Union{scalar_types,ZZRingElem}
+const scalar_types_extended = Union{scalar_types,IntegerUnion}

However, it creates a bunch of errors in the type tests that I don't know how to fix: oscar-system/Oscar.jl/actions/runs/12946690999/job/36111735653?pr=4444#step:8:746

Can you please look into it when you have the time (most likely not during the currently ongoing polymake developer meeting)? Many thanks in advance!

from #4445 (comment)

I am confused, as this PR does not touch scalar_types_extended ?!?

Anyway, the error in the linked log suggest that more things need to be changed if you adjust scalar_types_extended. Presumably if an Int is now used in places, them at some point some code may call parent on it, and this yields Integers{Int64}() -- a special placeholder parent for Julia integers that AA provides. And apparently this is then passed as first argument to point_vector which doesn't know about it.

Thanks, I am aware of the issue and will take a look.

@benlorenz
Copy link
Copy Markdown
Member

benlorenz commented Jan 31, 2025

The broadcast errors should be fixed now (and I fixed a small issue in the fan negation).

I think most of these constructions would benefit from adding non_redundant=true ? (Since they are using non-redundant data from the original objects)

@fingolfin
Copy link
Copy Markdown
Member

What the status of this PR?

@fingolfin fingolfin added triage and removed triage labels Jun 18, 2025
@YueRen
Copy link
Copy Markdown
Member Author

YueRen commented Sep 23, 2025

Sorry for sleeping on this PR, I will look into it and see whether it is mergeable.

@YueRen YueRen marked this pull request as ready for review September 23, 2025 14:11
Copy link
Copy Markdown
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

Looks good to me

Comment thread src/PolyhedralGeometry/PolyhedralComplex/standard_constructions.jl Outdated
Comment thread src/PolyhedralGeometry/PolyhedralComplex/standard_constructions.jl
Comment thread src/PolyhedralGeometry/PolyhedralComplex/standard_constructions.jl Outdated
Comment thread src/PolyhedralGeometry/PolyhedralComplex/standard_constructions.jl Outdated
Comment thread src/PolyhedralGeometry/PolyhedralComplex/standard_constructions.jl Outdated
Comment thread src/PolyhedralGeometry/PolyhedralComplex/standard_constructions.jl
Comment thread src/PolyhedralGeometry/PolyhedralComplex/standard_constructions.jl Outdated
Comment thread src/PolyhedralGeometry/PolyhedralFan/standard_constructions.jl Outdated
@fingolfin
Copy link
Copy Markdown
Member

The formatter complains now, but that could be addressed by running julia etc/format_code.jl (and even someone else can run it for you, @YueRen).

The main question is whether @lkastner and @benlorenz are happy with the the general content now, the formatting is an easy afterthought.

@YueRen
Copy link
Copy Markdown
Member Author

YueRen commented Oct 7, 2025

The formatter complains now, but that could be addressed by running julia etc/format_code.jl (and even someone else can run it for you, @YueRen).

The main question is whether @lkastner and @benlorenz are happy with the the general content now, the formatting is an easy afterthought.

I've run the formatting script and pushed the changes.

Copy link
Copy Markdown
Member

@lkastner lkastner left a comment

Choose a reason for hiding this comment

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

Thank you. We should probably expand transform on the polymake side though.

Comment thread test/PolyhedralGeometry/polyhedral_complex.jl Outdated
Comment thread test/PolyhedralGeometry/polyhedral_complex.jl Outdated
Comment thread test/PolyhedralGeometry/polyhedral_fan.jl Outdated
@benlorenz benlorenz changed the title PolyhedralGeometry: more operators on fans and complexes add transformations for fans and complexes Oct 10, 2025
@benlorenz benlorenz changed the title add transformations for fans and complexes add transformations for polyhedral fans and complexes Oct 10, 2025
@benlorenz benlorenz enabled auto-merge (squash) October 10, 2025 10:19
@benlorenz benlorenz merged commit 143e6d0 into master Oct 10, 2025
36 of 38 checks passed
@benlorenz benlorenz deleted the yr/PolyhedralOperators branch October 10, 2025 10:19
@benlorenz benlorenz added release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes and removed release notes: to be added PRs introducing changes that should be (but have not yet been) mentioned in the release notes triage labels Oct 10, 2025
@lgoettgens lgoettgens changed the title add transformations for polyhedral fans and complexes Add transformations for polyhedral fans and complexes Nov 20, 2025
@lgoettgens lgoettgens added the enhancement New feature or request label Nov 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes topic: polyhedral geometry Issue concerns polyhedral geometry code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants