yr/tropicalBasics

[YueRen]

Do not merge yet, this is just a test to see whether everything is fine.

[YueRen]

Some of the doctest is failing because OSCAR is printing out a set and the other of the element is different. Is there a way to take these documentations out of the doctest?

[fieker]

On Mon, Oct 09, 2023 at 03:19:23AM -0700, YueRen wrote: Some of the doctest is failing because OSCAR is printing out a set and the other of the element is different. Is there a way to take these documentations out of the doctest?

Yep. You can add regular expressions to add and ignore DocTestFilters = r"Group\(.*\]\)" e.g. in docs/src/NumberTheory/galois.md this removed the comparison of groups as the generators are randomized

…

-- Reply to this email directly or view it on GitHub: #2832 (comment) You are receiving this because you are subscribed to this thread. Message ID: ***@***.***>

[YueRen]

What do I have to do in order to get the tests running on github. I want to check whether there are any more failing tests.

[HereAround]

The tests will run once you fix the merge conflicts.

[YueRen]

So I will need to rebase it again? But how can there be merge conflicts? Did somebody change those files in the tropical folder in the last 48h?

[HereAround]

That is one way to fix it (and how I would do it). Presumably, you can also hit "resolve conflicts" below, but I have never done that before.

[HereAround]

So I will need to rebase it again? But how can there be merge conflicts? Did somebody change those files in the tropical folder in the last 48h?

The conflicts are in the following files:

 src/TropicalGeometry/initial.jl
src/TropicalGeometry/linear_space.jl
src/TropicalGeometry/valuation.jl
src/TropicalGeometry/variety.jl

The last commit that edited the file variety.jl is the following:

commit d15d9a8e0ac38d9707749c4aa3fb46812637e834
Author: Max Horn <[email protected]>
Date:   Tue Oct 10 08:21:16 2023 +0200

    Bump dependencies, adjust to changes (#2903)
    
    * Bump dependencies, adjust to changes
    
    * rename `mat` to `matrix`
    
    ---------
    
    Co-authored-by: Felix Röhrich <[email protected]>

(In case you wonder, I found this via git log -p <filepath>.)

[YueRen]

Got it, I was just majorly confused.

[YueRen]

There are still some tests failing, but I don't know why from the error messages. Can somebody give me a hint if there's still something wrong with my code? Otherwise, apart from some cosmetic changes the pull-request is ready to go.

[benlorenz]

There are still some tests failing, but I don't know why from the error messages. Can somebody give me a hint if there's still something wrong with my code? Otherwise, apart from some cosmetic changes the pull-request is ready to go.

It is still marked as a draft.
A large chunk of the file src/TropicalGeometry/curve.jl is commented, in particular the statement using RecipesBase which causes Aqua.jl to fail because we now have a unused dependency. What is the plan with this code? (There are large commented code blocks in other files as well)

[lgoettgens]

I think you removed the last instance of an using RecipesBase from Oscar in src/TropicalGeometry/curve.jl:6. So we no longer need that package as a dependency, and it can be thus removed from Project.toml.

[YueRen]

I keep getting the following error in the nightly test, is this caused by my changes? I haven't made any changes to runtests.jl.

[ Info: /home/runner/work/Oscar.jl/Oscar.jl/test/runtests.jl -- fixed SEED 42
ERROR: LoadError: TaskFailedException

    nested task error: 
GC: pause 148.68ms. collected 388.283483MB. incr 
Heap stats: bytes_mapped 512.12 MB, bytes_resident 323.12 MB,
heap_size 510.01 MB, heap_target 597.51 MB, Fragmentation 0.683
invalid method definition in Main: function Main.include must be explicitly imported to be extended
    Stacktrace:
     [1] top-level scope
       @ none:0
     [2] top-level scope
       @ none:1
     [3] eval
       @ Core ./boot.jl:398 [inlined]
     [4] (::Distributed.var"#172#174"{Module, Expr})()
       @ Distributed /opt/hostedtoolcache/julia/nightly/x64/share/julia/stdlib/v1.11/Distributed/src/macros.jl:233
Stacktrace:
 [1] sync_end(c::Channel{Any})
   @ Base ./task.jl:450
 [2] macro expansion
   @ ./task.jl:483 [inlined]
 [3] remotecall_eval(m::Module, procs::Vector{Int64}, ex::Expr)
   @ Distributed /opt/hostedtoolcache/julia/nightly/x64/share/julia/stdlib/v1.11/Distributed/src/macros.jl:219
 [4] top-level scope
   @ /opt/hostedtoolcache/julia/nightly/x64/share/julia/stdlib/v1.11/Distributed/src/macros.jl:203
 [5] include(fname::String)
   @ Base.MainInclude ./client.jl:495
 [6] top-level scope
   @ none:6
in expression starting at /home/runner/work/Oscar.jl/Oscar.jl/test/runtests.jl:85
ERROR: LoadError: Package Oscar errored during testing
Stacktrace:
 [1] pkgerror(msg::String)
   @ Pkg.Types /opt/hostedtoolcache/julia/nightly/x64/share/julia/stdlib/v1.11/Pkg/src/Types.jl:70
 [2] test(ctx::Pkg.Types.Context, pkgs::Vector{Pkg.Types.PackageSpec}; coverage::Bool, julia_args::Cmd, test_args::Cmd, test_fn::Nothing, force_latest_compatible_version::Bool, allow_earlier_backwards_compatible_versions::Bool, allow_reresolve::Bool)
   @ Pkg.Operations /opt/hostedtoolcache/julia/nightly/x64/share/julia/stdlib/v1.11/Pkg/src/Operations.jl:2020
 [3] test
   @ /opt/hostedtoolcache/julia/nightly/x64/share/julia/stdlib/v1.11/Pkg/src/Operations.jl:1901 [inlined]
 [4] test(ctx::Pkg.Types.Context, pkgs::Vector{Pkg.Types.PackageSpec}; coverage::Bool, test_fn::Nothing, julia_args::Vector{String}, test_args::Cmd, force_latest_compatible_version::Bool, allow_earlier_backwards_compatible_versions::Bool, allow_reresolve::Bool, kwargs::@Kwargs{io::Pkg.UnstableIO})
   @ Pkg.API /opt/hostedtoolcache/julia/nightly/x64/share/julia/stdlib/v1.11/Pkg/src/API.jl:444
 [5] test(pkgs::Vector{Pkg.Types.PackageSpec}; io::Pkg.UnstableIO, kwargs::@Kwargs{coverage::Bool, julia_args::Vector{String}, force_latest_compatible_version::Bool})
   @ Pkg.API /opt/hostedtoolcache/julia/nightly/x64/share/julia/stdlib/v1.11/Pkg/src/API.jl:159
 [6] test(; name::Nothing, uuid::Nothing, version::Nothing, url::Nothing, rev::Nothing, path::Nothing, mode::Pkg.Types.PackageMode, subdir::Nothing, kwargs::@Kwargs{coverage::Bool, julia_args::Vector{String}, force_latest_compatible_version::Bool})
   @ Pkg.API /opt/hostedtoolcache/julia/nightly/x64/share/julia/stdlib/v1.11/Pkg/src/API.jl:174
 [7] top-level scope
   @ ~/work/_actions/julia-actions/julia-runtest/latest/test_harness.jl:15
 [8] include(fname::String)
   @ Base.MainInclude ./client.jl:495
 [9] top-level scope
   @ none:1
in expression starting at /home/runner/work/_actions/julia-actions/julia-runtest/latest/test_harness.jl:7
Error: Process completed with exit code 1.

[YueRen]

@bschroter @lkastner The pull request is ready to be reviewed. If you want, I can give you a brief rundown over Zoom to help you get started (maybe around the OSCAR meeting this Friday?).

[benlorenz]

A first round of comments.

[benlorenz]

I will provide a PR which does this properly.

[YueRen]

That's great! If you do, can you please add an option to disable the checks whether the input polyhedra actually form a polyhedral complex? I should always be calling it with a set of maximal polyhedra of a polyhedral complex, no checks required.

[YueRen]

Some of the tests that are currently failing are in the matroid package. I don't think they are related to my changes.

[codecov]

Codecov Report

Merging #2832 (6053b11) into master (a179656) will decrease coverage by 0.10%.
Report is 7 commits behind head on master.
The diff coverage is 53.12%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2832      +/-   ##
==========================================
- Coverage   80.43%   80.33%   -0.10%     
==========================================
  Files         506      522      +16     
  Lines       70074    70150      +76     
==========================================
- Hits        56365    56357       -8     
- Misses      13709    13793      +84     

Files	Coverage Δ
src/Serialization/TropicalGeometry.jl	93.33% <100.00%> (+21.90%)	⬆️
src/TropicalGeometry/TropicalGeometry.jl	100.00% <100.00%> (+9.52%)	⬆️
src/TropicalGeometry/groebner_fan.jl	87.19% <50.00%> (ø)
src/TropicalGeometry/matrix.jl	77.77% <77.77%> (ø)
src/TropicalGeometry/semiring_map.jl	97.10% <97.10%> (ø)
src/TropicalGeometry/initial.jl	78.26% <78.26%> (+9.51%)	⬆️
src/TropicalGeometry/curve.jl	75.00% <75.00%> (-4.70%)	⬇️
src/TropicalGeometry/groebner_basis.jl	90.90% <90.80%> (+0.28%)	⬆️
src/TropicalGeometry/linear_space.jl	74.76% <74.76%> (-21.39%)	⬇️
src/TropicalGeometry/hypersurface.jl	61.53% <60.52%> (-21.16%)	⬇️
... and 5 more

... and 24 files with indirect coverage changes

[YueRen]

All required tests are now running through, let me know if you need any help with the reviews @bschroter @lkastner. I'd really like to get this merged sooner than later so my other collaborators can start building on top of it.

[fingolfin]

Unfortunately this has a merge conflict now :-/

[YueRen]

Unfortunately this has a merge conflict now :-/

It doesn't look too hard to fix. I will get to it once the reviews are done.

[bschroter]

I had a quick look at the tropical functionality (mainly from a users point of view and I only briefly looked at the code).

1. There are some larger parts of code just as a comment (see line 570 of variety.jl)
   Maybe this requires first a little bit of clean up.

2. Also it says `tropical_variety is currently under development' does that mean this should be an experimental package for the moment?

3. It is unclear to me whether the debug files should be part of the code base or not.

4. Should be functionality as Gröbner fans really be hidden in the tropical source code?

5. It might be confusing that tropical_variety returns an array while the other constructors as tropical_linear_space or tropical_hypersurface, tropical_curve return the variety directly.

6. [maybe more an enhancement] Should it be possible to intersect a tropical_linear_space with a tropical_hypersurface etc.?

[YueRen]

Thanks for the comments @bschroter.

1. There are some larger parts of code just as a comment (see line 570 of variety.jl)
   Maybe this requires first a little bit of clean up.

The code is kind of important, it is the beginning of the code for computing arbitrary tropicalizations. However, there are still a couple of hard-to-fix bugs that need to be ironed out.

2. Also it says `tropical_variety is currently under development' does that mean this should be an experimental package for the moment?

The functionality that is there works or should work. But it is not feature complete yet.

4. Should be functionality as Gröbner fans really be hidden in the tropical source code?

No, it should be documented. I'll see whether I can add some quick documentation for it.

5. It might be confusing that tropical_variety returns an array while the other constructors as tropical_linear_space or tropical_hypersurface, tropical_curve return the variety directly.

True, maybe tropical_variety should just assume that the input is a prime ideal then and introduce a new function factorize_and_tropicalize or something similar. Or alternatively rename the function to tropical_varieties instead.

6. [maybe more an enhancement] Should it be possible to intersect a tropical_linear_space with a tropical_hypersurface etc.?

If there is a quick algorithm for doing so, sure! Otherwise, their intersection should fall back to the general intersection of tropical varieties.

[YueRen]

@lkastner @benlorenz @thofma Some tests in PolyhedralGeometry are failing for me, do you know what may be causing them?
https://github.com/oscar-system/Oscar.jl/actions/runs/6982130645/job/19000676739?pr=2832

It looks Nemo related to me.

[thofma]

Without being at a computer, I am not aware of any change that might have caused this. @benlorenz did something change on the polyhedral side?

[benlorenz]

The backtrace for the failing polyhedral tests contains:

 Oscar ~/work/Oscar.jl/Oscar.jl/src/TropicalGeometry/variety.jl:122

You need to remove the custom polyhedral_complex(Vector{Polyhedron}) function (https://github.com/oscar-system/Oscar.jl/pull/2832/files#diff-0379b0f2b166f68b219d831526ab1e7c3e0894abb0f5db4c14b1cadb509e0ed1R109), this overrides the functions that I added recently but fails for non-rational objects.

See #2959 (comment) for details on the functions I added.

[YueRen]

Okay, all issues seem to be fixed. The pull request is ready to be merged as far as I am concerned.