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

first try at tropical prevarieties #4703

Merged
merged 20 commits into from
Mar 25, 2025

Conversation

danteluber
Copy link
Contributor

@danteluber danteluber commented Mar 5, 2025

Want to introduce methods to intersect tropical hypersurfaces and produce topical prevarieties from finite collections of polynomials.

Follows discussion with @lkastner

@YueRen Interested in your thoughts as well

@danteluber danteluber force-pushed the tropical_prevariety branch from 2d58662 to 6d847ee Compare March 5, 2025 21:09
@YueRen
Copy link
Member

YueRen commented Mar 7, 2025

Hi @danteluber,

Thanks for the pr draft! Would it be possible to do the following changes:

  1. Wrap Polymake.fan.common_refinement for OSCAR, so that users can use common_refinement(...) for arbitrary polyhedral complexes.
  2. Make the tropical_prevariety function rely on it.

(Number 1. can be its own separate pull-request, if you want to tackle it step-by-step)

@YueRen
Copy link
Member

YueRen commented Mar 7, 2025

Oh, so there was a common_refinement already? That is great to know.

As for the remaining changes, you can construct tropical hypersurfaces from polynomials using tropical_hypersurface(f), so there is no need to define your own functions unless there is a problem with the existing ones.

@danteluber danteluber marked this pull request as ready for review March 13, 2025 08:25
Copy link
Member

@YueRen YueRen left a comment

Choose a reason for hiding this comment

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

Hi Dante, thanks again for your effort! I just have two final requests (see enclosed).



#Default to min convention
tropical_prevariety(A::Vector{<:MPolyRingElem}) = tropical_prevariety(min, A)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
tropical_prevariety(A::Vector{<:MPolyRingElem}) = tropical_prevariety(min, A)
function tropical_prevariety(F::Vector{<:MPolyRingElem}, nu::TropicalSemiringMap=tropical_semiring_map(coefficient_ring(first(F))))
return tropical_prevariety(tropical_polynomial.(F,Ref(nu))
end

I suggest adding a second optional TropicalSemiringMap that encodes a valuation + convention (default: trivial valuation, min convention). This allows users working over fields to create tropical hypersurfaces with respect to non-trivial valuations.

Comment on lines 304 to 310
function tropical_prevariety(convention::minmax, A::Vector{<:MPolyRingElem}) where {minmax<:Union{typeof(min),typeof(max)}}
length(A) > 0 || error("Empty array.")
nu = tropical_semiring_map(coefficient_ring(A[1]), convention)
ftrops = [tropical_polynomial(f, nu) for f in A]
HTA = [tropical_hypersurface(f) for f in ftrops]
return reduce(common_refinement, [h.polyhedralComplex for h in HTA])
end
Copy link
Member

@YueRen YueRen Mar 14, 2025

Choose a reason for hiding this comment

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

Suggested change
function tropical_prevariety(convention::minmax, A::Vector{<:MPolyRingElem}) where {minmax<:Union{typeof(min),typeof(max)}}
length(A) > 0 || error("Empty array.")
nu = tropical_semiring_map(coefficient_ring(A[1]), convention)
ftrops = [tropical_polynomial(f, nu) for f in A]
HTA = [tropical_hypersurface(f) for f in ftrops]
return reduce(common_refinement, [h.polyhedralComplex for h in HTA])
end

I would suggest removing this, as it is superseded by the suggested function above.

Copy link
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.

Looks good to me.

Copy link
Member

@benlorenz benlorenz left a comment

Choose a reason for hiding this comment

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

Please add the function to some documentation page, i.e. an .md file in docs/src.

Polyhedral complex in ambient dimension 10
```
"""
#Input array of tropical polynomials
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#Input array of tropical polynomials

This line needs to be removed for the docstring to work.

The doctest is also not run currently.

Comment on lines +310 to +312
#Give input as tropical polynomials
Dr25 = tropical_prevariety(tropical_polynomial.([f for f in gens(Gr25)]))
Polyhedral complex in ambient dimension 10
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#Give input as tropical polynomials
Dr25 = tropical_prevariety(tropical_polynomial.([f for f in gens(Gr25)]))
Polyhedral complex in ambient dimension 10
julia> Dr25 = tropical_prevariety(tropical_polynomial.([f for f in gens(Gr25)])) # Give input as tropical polynomials
Polyhedral complex in ambient dimension 10

There is a julia> missing, also I don't think comments like this before the julia> lines will work, we do have it like this in other cases so I added this as a suggestion:

julia> some + code   # some explanation

Comment on lines +303 to +304
#Compute with respect to max convention
julia> nu = tropical_semiring_map(QQ,max)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#Compute with respect to max convention
julia> nu = tropical_semiring_map(QQ,max)
julia> nu = tropical_semiring_map(QQ,max) #Compute with respect to max convention

Comment on lines +286 to +287
#Compute Dressian without specified tropicalization map.
julia> Dr25 = tropical_prevariety(gens(Gr25))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#Compute Dressian without specified tropicalization map.
julia> Dr25 = tropical_prevariety(gens(Gr25))
julia> Dr25 = tropical_prevariety(gens(Gr25)) #Compute Dressian without specified tropicalization map.

#

@doc raw"""
tropical_prevariety(F::Vector{MPolyRingElem},nu::TropicalSemiringMap)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
tropical_prevariety(F::Vector{MPolyRingElem},nu::TropicalSemiringMap)
tropical_prevariety(F::Vector{MPolyRingElem},nu::TropicalSemiringMap)

This needs to be indented by 4 spaces.

@lkastner lkastner merged commit 1fe2f79 into oscar-system:master Mar 25, 2025
31 of 32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants