Fix tropical_variety_zerodimensional

[YueRen]

No description provided.

[fingolfin]

We need someone to review this, perhaps someone from Berlin (maybe @micjoswig @lkastner @benlorenz have a suggestion who could do it?)

[lkastner]

Ping @danteluber . Could you review this?

[YueRen]

@oliverclarke8787 Could you please review the pull request?

[lkastner]

Please add some documentation and tests. Currently this seems not to be covered and a proper review is much harder without an example.

[danteluber]

@YueRen My only input for the moment is that maybe the first thing it should do is verify that the input ideal is indeed 0 dimensional. I acknowledge that this is an obvious requirement from the name, but it can be informative if something goes wrong at the step where this function is used as part of a larger computation. The error number_field returns if you use an ideal that isn't 0 dimensional doesn't make this obvious either.

Also, does this function depend on the tropicalization map coming from a nontrivial valuation? In particular, the step where the padic_field is constructed? If so,maybe this should also be verified at the beginning.

[YueRen]

Please add some documentation and tests. Currently this seems not to be covered and a proper review is much harder without an example.

I've added a docstring now, hope this also means that it is covered in the doctests:

help?> Oscar.tropical_variety_zero_dimensional
  │ Warning
  │
  │  The following bindings may be internal; they may change or be removed in future
  │  versions:
  │
  │    •  Oscar.tropical_variety_zero_dimensional

  Oscar.tropical_variety_zerodimensional(I::MPolyIdeal,nu::TropicalSemiringMap{QQField,ZZRingElem,<:Union{typeof(min),typeof(max)}}; precision::Int=64)

  Internal function for computing zero-dimensional tropical varieties over p-adic numbers
  via finite precision Eigenvalue computation. Assumes without test that I is
  zero-dimensional.

  Examples
  ≡≡≡≡≡≡≡≡

  julia> R,(x1,x2,x3) = polynomial_ring(QQ,3);
  
  julia> I = ideal([28*x3^2 - 1*x3 - 1,
                    2*x2 - x3,
                    2*x1 - x2]);
  
  julia> nu = tropical_semiring_map(QQ,2)
  Map into Min tropical semiring encoding the 2-adic valuation on Rational field
  
  julia> TropI = Oscar.tropical_variety_zero_dimensional(I,nu)
  Min tropical variety
  
  julia> vertices(TropI)
  2-element SubObjectIterator{PointVector{QQFieldElem}}:
   [-2, -1, 0]
   [-4, -3, -2]
  
  julia> nu = tropical_semiring_map(QQ,3,max)
  Map into Max tropical semiring encoding the 3-adic valuation on Rational field
  
  julia> TropI = Oscar.tropical_variety_zero_dimensional(I,nu)
  Max tropical variety
  
  julia> vertices(TropI)
  1-element SubObjectIterator{PointVector{QQFieldElem}}:
   [0, 0, 0]
  

[YueRen]

@YueRen My only input for the moment is that maybe the first thing it should do is verify that the input ideal is indeed 0 dimensional. I acknowledge that this is an obvious requirement from the name, but it can be informative if something goes wrong at the step where this function is used as part of a larger computation. The error number_field returns if you use an ideal that isn't 0 dimensional doesn't make this obvious either.

Also, does this function depend on the tropicalization map coming from a nontrivial valuation? In particular, the step where the padic_field is constructed? If so,maybe this should also be verified at the beginning.

@danteluber Hi Dante, thanks for looking into it and apologies for the late reply.

1. Yes, the ideal needs to be 0 dimensional and the function will error if it is not. However, the function is also an internnal non-user-facing function, so I think it is fine not testing that (especially given how expensive dimensionality tests are).

2. Yes, the tropicalization map has to be from a p-adic valuation, that is enforced by the nu::TropicalSemiringMap{QQField,ZZRingElem,<:Union{typeof(min),typeof(max)}} in the function signature:

julia> nu = tropical_semiring_map(QQ)
Map into Min tropical semiring encoding the trivial valuation on Rational field

julia> typeof(nu)
TropicalSemiringMap{QQField, Nothing, typeof(min)}

julia> nu = tropical_semiring_map(QQ,2)
Map into Min tropical semiring encoding the 2-adic valuation on Rational field

julia> typeof(nu)
TropicalSemiringMap{QQField, ZZRingElem, typeof(min)}

Either way, sorry for the confusion. As @lkastner pointed out, this whole thing could have been avoided if I had added a docstring in the first place, which is the sensible thing to do if I expect people to review it.

[benlorenz]

You need to add the signature to one of the .md files, otherwise it won't show up in the online-docs.

[YueRen]

There were some failing test were that are unrelated to my code. I've rebased the PR to the latest master, hopefully that fixes the issue.

[YueRen]

You need to add the signature to one of the .md files, otherwise it won't show up in the online-docs.

Thanks for the tip. I think I'm fine with the docstring not showing up in the online-docs. I still consider it very experimental for now, and only exists because one of Bernd's students needs it. In the future, it should be called from some general user-facing tropicalization function.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.94%. Comparing base (08c390a) to head (04376fa).
Report is 8 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4838      +/-   ##
==========================================
+ Coverage   84.87%   84.94%   +0.06%     
==========================================
  Files         683      683              
  Lines       91746    91741       -5     
==========================================
+ Hits        77872    77925      +53     
+ Misses      13874    13816      -58     

Files with missing lines	Coverage Δ
src/TropicalGeometry/variety.jl	90.78% <100.00%> (+40.47%)	⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Thanks for adding these things.