Prefer symbols over strings as indeterminants #4158
Conversation
lgoettgens
force-pushed
the
lg/prefer-symbols
branch
from
c21d8a0
to
59b30e5
Compare
lgoettgens
force-pushed
the
lg/prefer-symbols
branch
from
8acda26
to
1f14c6e
Compare
lgoettgens
force-pushed
the
lg/prefer-symbols
branch
from
1f14c6e
to
4799631
Compare
|
@benlorenz is this failure something similar to what you work around in #4153 or is a genuine failure? alternative inputs: Test Failed at /Users/aaruni/Desktop/oscar-runners/runner-1/_work/Oscar.jl/Oscar.jl/test/PolyhedralGeometry/subdivision_of_points.jl:17
Expression: collect(maximal_cells(square_by_incidence)) == collect(maximal_cells(square_by_weights))
Evaluated: [[1, 2, 3], [2, 3, 4]] == [[2, 3, 4], [1, 2, 3]] |
| @@ -61,7 +61,7 @@ function global_tate_model(base::NormalToricVariety, | |||
| vs2 = collect(keys(defining_section_parametrization)) | |||
| @req all(in(["a1", "a2", "a3", "a4", "a6"]), vs2) "Only the Tate sections a1, a2, a3, a4, a6 must be parametrized" | |||
|
|
|||
| gens_base_names = [string(g) for g in gens(cox_ring(base))] | |||
| gens_base_names = [string(g) for g in symbols(cox_ring(base))] | |||
There was a problem hiding this comment.
@HereAround pinging you to make you aware that symbols(::MPolyRing) exists. In many of these cases you probably don't even need to convert to strings afterwards, but I kept it like this if I didn't see an obvious way to change the below code
There was a problem hiding this comment.
Actually in this particular case it seems straightforward, as the only place using this seems to be the next line?
I only noticed (and now fixed) those in |
lgoettgens
force-pushed
the
lg/prefer-symbols
branch
from
8d55dad
to
15b155b
Compare
| @test collect(maximal_cells(square_by_incidence)) == | ||
| collect(maximal_cells(square_by_weights)) | ||
| @test issetequal(collect(maximal_cells(square_by_incidence)), | ||
| collect(maximal_cells(square_by_weights))) |
There was a problem hiding this comment.
@benlorenz I added this already here to try to get CI green eventually
There was a problem hiding this comment.
Ok, thanks! The collect should be unnecessary now but I can take care of that later.
For the cases in polyhedron.jl a rerun usually fixed it.
lgoettgens
force-pushed
the
lg/prefer-symbols
branch
2 times, most recently
from
0784fd0
to
ef891b7
Compare
Co-authored-by: Max Horn <[email protected]>
lgoettgens
force-pushed
the
lg/prefer-symbols
branch
from
ef891b7
to
fdbbec1
Compare
lgoettgens
force-pushed
the
lg/prefer-symbols
branch
from
fdbbec1
to
6daf07b
Compare
There was a problem hiding this comment.
The Algebraic Geometry/Commutative Algebra part looks good provided tests pass -- with the exception that I did not look at toric stuff (because you already pinged @HereAround ) and IntersectionTheory should be pinged as well.
lgoettgens
force-pushed
the
lg/prefer-symbols
branch
from
6daf07b
to
f3d075e
Compare
lgoettgens
added
the
optimization
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4158 +/- ##
==========================================
- Coverage 84.70% 84.59% -0.11%
==========================================
Files 628 631 +3
Lines 84449 84811 +362
==========================================
+ Hits 71529 71749 +220
- Misses 12920 13062 +142
|
| @@ -525,7 +525,7 @@ function _construct_literature_model_over_arbitrary_base(model_dict::Dict{String | |||
| elseif model_dict["model_descriptors"]["type"] == "hypersurface" | |||
|
|
|||
| # Extract base variable names | |||
| auxiliary_base_vars = [string(g) for g in gens(auxiliary_base_ring)] | |||
| auxiliary_base_vars = [string(g) for g in symbols(auxiliary_base_ring)] | |||
|
|
|||
| # Extract fiber ambient space | |||
| rays = [[a for a in b] for b in model_dict["model_data"]["fiber_ambient_space_rays"]] | |||
There was a problem hiding this comment.
Off-topic for this PR, but: I wonder if [a for a in b] shouldn't be replaced by collect(b) here and elsewhere? This line could perhaps even become
| rays = [[a for a in b] for b in model_dict["model_data"]["fiber_ambient_space_rays"]] | |
| rays = collect.(model_dict["model_data"]["fiber_ambient_space_rays"]) |
(Or maybe not, I did not try it and depending on what the code does this may or may not work, and it may or may not affect performance and allocations in either a good or bad way). So, just something to be potentially aware of, @HereAround
| @@ -61,7 +61,7 @@ function global_tate_model(base::NormalToricVariety, | |||
| vs2 = collect(keys(defining_section_parametrization)) | |||
| @req all(in(["a1", "a2", "a3", "a4", "a6"]), vs2) "Only the Tate sections a1, a2, a3, a4, a6 must be parametrized" | |||
|
|
|||
| gens_base_names = [string(g) for g in gens(cox_ring(base))] | |||
| gens_base_names = [string(g) for g in symbols(cox_ring(base))] | |||
There was a problem hiding this comment.
Actually in this particular case it seems straightforward, as the only place using this seems to be the next line?
| @@ -144,7 +144,7 @@ Global Tate model over a not fully specified base | |||
| function global_tate_model(auxiliary_base_ring::MPolyRing, auxiliary_base_grading::Matrix{Int64}, d::Int, ais::Vector{T}) where {T<:MPolyRingElem} | |||
|
|
|||
| # Execute consistency checks | |||
| gens_base_names = [string(g) for g in gens(auxiliary_base_ring)] | |||
| gens_base_names = [string(g) for g in symbols(auxiliary_base_ring)] | |||
There was a problem hiding this comment.
Here we can't (yet) change to just using symbols directly because this vector is passed to _construct_generic_sample which currently expects a Vector{Symbol}. But perhaps this could be changed in the future. Alas this doesn't look like a function that is called millions of time, so may not be worth it (similar in other cases -- use your own judgement)
Co-authored-by: Max Horn <[email protected]>
|
This is now good to go from my POV |
polynomial_ringused to get discarded and a line latergenswas called@lkastner @HereAround could you have a look if the attributes(moved to #4159)coordinate_namesandcoordinate_names_of_torusin the toric varieties could be changed toVector{Symbol}instead ofVector{String}to reduce the allocations from copying this around and passing it topolynomial_ringetc?