Tropical variety overhaul

[YueRen]

New feature: Interface to tropicalVariety in Singular for tropicalization of ideals.

[fingolfin]

Unfortunately now has a merged conflict in src/TropicalGeometry/variety.jl

[YueRen]

A book test (from my chapter) is failing due to a syntax change in tropical_variety (it used to return a list of TropicalVariety, not it simply returns one TropicalVariety), is it okay to ignore book tests or do we still want to adhere to them strictly?

[joschmitt]

it used to return a list of TropicalVariety, not it simply returns one TropicalVariety

This sounds like a breaking change to me which we cannot do in 1.*?

[benlorenz]

A book test (from my chapter) is failing due to a syntax change in tropical_variety (it used to return a list of TropicalVariety, not it simply returns one TropicalVariety), is it okay to ignore book tests or do we still want to adhere to them strictly?

Unless it is a clear bugfix, the input in the booktests should keep working for all 1.x versions. Only the printing is allowed to change.

Looking at the log there is also an error and not an output change:

  julia> TropV = tropical_variety(I,nu)
- 2-element Vector{TropicalVariety}:
-  Min tropical variety
-  Min tropical variety
+ ERROR: MethodError: no method matching tropical_variety_prime_singular(::MPolyIdeal{…}, ::TropicalSemiringMap{…}; weighted_polyhedral_complex_only::Bool)
+ 
+ Closest candidates are:
+   tropical_variety_prime_singular(::MPolyIdeal, !Matched::TropicalSemiringMap{QQField, ZZRingElem, <:Union{typeof(max), typeof(min)}}; weighted_polyhedral_complex_only)
+    @ Oscar ~/work/Oscar.jl/Oscar.jl/src/TropicalGeometry/variety_prime.jl:42
+   tropical_variety_prime_singular(::MPolyIdeal, !Matched::TropicalSemiringMap{QQField, Nothing, <:Union{typeof(max), typeof(min)}}; weighted_polyhedral_complex_only)
+    @ Oscar ~/work/Oscar.jl/Oscar.jl/src/TropicalGeometry/variety_prime.jl:23
+ 
+ Stacktrace:
+  [1] tropical_variety_prime(I::MPolyIdeal{…}, nu::TropicalSemiringMap{…}; weighted_polyhedral_complex_only::Bool)
+    @ Oscar ~/work/Oscar.jl/Oscar.jl/src/TropicalGeometry/variety_prime.jl:18
+  [2] tropical_variety(I::MPolyIdeal{…}, nu::TropicalSemiringMap{…}; weighted_polyhedral_complex_only::Bool)
+    @ Oscar ~/work/Oscar.jl/Oscar.jl/src/TropicalGeometry/variety.jl:209
+  [3] tropical_variety(I::MPolyIdeal{…}, nu::TropicalSemiringMap{…})
+    @ Oscar ~/work/Oscar.jl/Oscar.jl/src/TropicalGeometry/variety.jl:189
+  [4] top-level scope
+    @ REPL[51]:1
+ Some type information was truncated. Use `show(err)` to see complete types.

[YueRen]

it used to return a list of TropicalVariety, not it simply returns one TropicalVariety

This sounds like a breaking change to me which we cannot do in 1.*?

Yes, I agree that it is a breaking change. So it will have to wait until 2.x then?

[joschmitt]

it used to return a list of TropicalVariety, not it simply returns one TropicalVariety

This sounds like a breaking change to me which we cannot do in 1.*?

Yes, I agree that it is a breaking change. So it will have to wait until 2.x then?

Sounds like it, but in the end this is not for me to decide. Could you provide a reasonable version of this pull request without the breaking changes? By "reasonable", I mean both in terms of functionality but also the work that needs to be done.

[YueRen]

Yeah, the error is my bad. I'll try to fix it and produce something mergable this week, whether it is for 1.x or 2.x.

[YueRen]

There is still an error with a book chapter, is that due to an output change? The way tropical_variety was used in the book chapter should now trigger a custom error.

That being said, as this is a breaking change, I should probably add suggestions to the error message what the user should be using instead of tropical_variety.

[joschmitt]

The actual error is in line 374 of the log (https://github.com/oscar-system/Oscar.jl/actions/runs/11896996464/job/33150243661?pr=4061#step:10:378):

  julia> TropV = tropical_variety(I,nu)
- 2-element Vector{TropicalVariety}:
-  Min tropical variety
-  Min tropical variety
+ ERROR: ArgumentError: Only primary ideals supported.
+ Stacktrace:
+  [1] macro expansion
+    @  AbstractAlgebra [inlined]
+  [2] tropical_variety(I::MPolyIdeal{…}, nu::TropicalSemiringMap{…}; weighted_polyhedral_complex_only::Bool, check::Bool)
+    @ Oscar ~/work/Oscar.jl/Oscar.jl/src/TropicalGeometry/variety.jl:211
+  [3] tropical_variety(I::MPolyIdeal{…}, nu::TropicalSemiringMap{…})
+    @ Oscar ~/work/Oscar.jl/Oscar.jl/src/TropicalGeometry/variety.jl:191
+  [4] top-level scope
+    @ REPL[51]:1
+ Some type information was truncated. Use `show(err)` to see complete types.
  
  julia> dim.(TropV)
- 2-element Vector{Int64}:
-  2
-  1
+ ERROR: UndefVarError: `TropV` not defined
+ Stacktrace:
+  [1] top-level scope
+    @ REPL[52]:1"""

ERROR: ArgumentError: Only primary ideals supported. doesn't sound good to me...

[YueRen]

The actual error is in line 374 of the log (https://github.com/oscar-system/Oscar.jl/actions/runs/11896996464/job/33150243661?pr=4061#step:10:378):

  julia> TropV = tropical_variety(I,nu)
- 2-element Vector{TropicalVariety}:
-  Min tropical variety
-  Min tropical variety
+ ERROR: ArgumentError: Only primary ideals supported.
+ Stacktrace:
+  [1] macro expansion
+    @  AbstractAlgebra [inlined]
+  [2] tropical_variety(I::MPolyIdeal{…}, nu::TropicalSemiringMap{…}; weighted_polyhedral_complex_only::Bool, check::Bool)
+    @ Oscar ~/work/Oscar.jl/Oscar.jl/src/TropicalGeometry/variety.jl:211
+  [3] tropical_variety(I::MPolyIdeal{…}, nu::TropicalSemiringMap{…})
+    @ Oscar ~/work/Oscar.jl/Oscar.jl/src/TropicalGeometry/variety.jl:191
+  [4] top-level scope
+    @ REPL[51]:1
+ Some type information was truncated. Use `show(err)` to see complete types.
  
  julia> dim.(TropV)
- 2-element Vector{Int64}:
-  2
-  1
+ ERROR: UndefVarError: `TropV` not defined
+ Stacktrace:
+  [1] top-level scope
+    @ REPL[52]:1"""

ERROR: ArgumentError: Only primary ideals supported. doesn't sound good to me...

Right, I have fixed the error message. It should now tell users to use tropical_varieties instead (or disable the check if they know what they are doing).

[fingolfin]

I think the point @joschmitt wanted to make is this: would it be possible for you with reasonable effort (i.e. we are not asking you to spend days on this; hopefully this is something that can be done quickly) to change this PR so that it is not breaking the booktests? Presumably that would mean letting tropical_variety again return a vector with multiple results in that case. You could still also print a warning (printing changes are allowed) stating that this is deprecated and what should be called instead.

This way this PR could already be merged and the bulk of the changes would be in there. And then you could open a separate PR which probably would be very short and which turns the warning into an error again. That PR then could be added to a milestone for Oscar 2.0.

[YueRen]

I see, that is what @joschmitt meant. I can do that, but I'll take care of my two workshop PRs first.