Prefer symbols over strings as indeterminants #4158

lgoettgens · 2024-09-27T13:30:12Z

adds a stylge guide entry (as wished for in Prefer symbols over strings in polynomial_ring #4133 (comment) by @lkastner)
updates many more non-trivial places, including some small optimizations where the second return value of polynomial_ring used to get discarded and a line later gens was called

@lkastner @HereAround could you have a look if the attributes coordinate_names and coordinate_names_of_torus in the toric varieties could be changed to Vector{Symbol} instead of Vector{String} to reduce the allocations from copying this around and passing it to polynomial_ring etc? (moved to #4159)

experimental/FTheoryTools/src/auxiliary.jl

lgoettgens · 2024-09-27T14:58:11Z

@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]]

https://github.com/oscar-system/Oscar.jl/actions/runs/11072577996/job/30767135768?pr=4158#step:9:1856

lgoettgens · 2024-09-27T15:20:45Z

experimental/FTheoryTools/src/TateModels/constructors.jl

@@ -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))]


@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

Actually in this particular case it seems straightforward, as the only place using this seems to be the next line?

benlorenz · 2024-09-27T15:22:38Z

@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]]
oscar-system/Oscar.jl/actions/runs/11072577996/job/30767135768?pr=4158#step:9:1856

I only noticed (and now fixed) those in polyhedron.jl so far but this is similar and I will do another PR soon.

lgoettgens · 2024-09-27T15:28:51Z

test/PolyhedralGeometry/subdivision_of_points.jl

- @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)))


@benlorenz I added this already here to try to get CI green eventually

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.

Co-authored-by: Max Horn <max@quendi.de>

afkafkafk13

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.

experimental/BasisLieHighestWeight/src/WeylPolytope.jl

codecov · 2024-09-30T10:00:55Z

Codecov Report

Attention: Patch coverage is 93.84615% with 4 lines in your changes missing coverage. Please review.

Project coverage is 84.59%. Comparing base (32a38b3) to head (61545aa).
Report is 15 commits behind head on master.

Files with missing lines	Patch %	Lines
experimental/FTheoryTools/src/auxiliary.jl	81.81%	2 Missing ⚠️
experimental/IntersectionTheory/src/Bott.jl	50.00%	1 Missing ⚠️
...metricIntersections/src/symmetric_grassmannians.jl	0.00%	1 Missing ⚠️

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

Files with missing lines	Coverage Δ
...rimental/BasisLieHighestWeight/src/WeylPolytope.jl	`100.00% <100.00%> (ø)`
.../FTheoryTools/src/AbstractFTheoryModels/methods.jl	`78.27% <100.00%> (ø)`
...TheoryTools/src/HypersurfaceModels/constructors.jl	`96.72% <100.00%> (ø)`
...tal/FTheoryTools/src/HypersurfaceModels/methods.jl	`100.00% <100.00%> (ø)`
.../FTheoryTools/src/LiteratureModels/constructors.jl	`93.81% <100.00%> (ø)`
...mental/FTheoryTools/src/TateModels/constructors.jl	`97.46% <100.00%> (ø)`
...xperimental/FTheoryTools/src/TateModels/methods.jl	`66.29% <100.00%> (ø)`
...FTheoryTools/src/WeierstrassModels/constructors.jl	`95.65% <100.00%> (ø)`
...ntal/FTheoryTools/src/WeierstrassModels/methods.jl	`100.00% <100.00%> (ø)`
...erimental/FTheoryTools/test/hypersurface_models.jl	`100.00% <ø> (ø)`
... and 29 more

... and 41 files with indirect coverage changes

fingolfin · 2024-10-06T19:51:32Z

experimental/FTheoryTools/src/LiteratureModels/constructors.jl

@@ -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"]]


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

Suggested change

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

fingolfin · 2024-10-06T19:54:20Z

experimental/FTheoryTools/src/TateModels/constructors.jl

@@ -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))]


Actually in this particular case it seems straightforward, as the only place using this seems to be the next line?

experimental/FTheoryTools/src/TateModels/constructors.jl

fingolfin · 2024-10-06T20:17:03Z

experimental/FTheoryTools/src/TateModels/constructors.jl

@@ -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)]


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 <max@quendi.de>

experimental/FTheoryTools/src/TateModels/constructors.jl

lgoettgens · 2024-10-07T09:56:16Z

This is now good to go from my POV

lgoettgens force-pushed the lg/prefer-symbols branch from c21d8a0 to 59b30e5 Compare September 27, 2024 13:43

fingolfin reviewed Sep 27, 2024

View reviewed changes

experimental/FTheoryTools/src/auxiliary.jl Outdated Show resolved Hide resolved

lgoettgens force-pushed the lg/prefer-symbols branch from 8acda26 to 1f14c6e Compare September 27, 2024 14:29

lgoettgens mentioned this pull request Sep 27, 2024

Symbols vs. strings in toric varieties #4159

Open

lgoettgens force-pushed the lg/prefer-symbols branch from 1f14c6e to 4799631 Compare September 27, 2024 14:34

lgoettgens commented Sep 27, 2024

View reviewed changes

lgoettgens force-pushed the lg/prefer-symbols branch from 8d55dad to 15b155b Compare September 27, 2024 15:27

lgoettgens commented Sep 27, 2024

View reviewed changes

lgoettgens force-pushed the lg/prefer-symbols branch 2 times, most recently from 0784fd0 to ef891b7 Compare September 27, 2024 16:20

lgoettgens and others added 9 commits September 27, 2024 18:21

Add "prefer symbols" paragraph to styleguide

0c1b088

Prefer symbols over strings in more non-trivial polynomial_ring calls

fdcf7ac

more trivial cases

be442cb

Update experimental/FTheoryTools/src/auxiliary.jl

e4df886

Co-authored-by: Max Horn <max@quendi.de>

more changes around projectivization

379f2df

fixes

bc56933

prefer symbols(::Ring) over string.(gens(::Ring)) where possible

3fa4532

Use isdisjoint

124b1b8

Adjust flaky test

658e5ea

lgoettgens force-pushed the lg/prefer-symbols branch from ef891b7 to fdbbec1 Compare September 27, 2024 16:35

lgoettgens marked this pull request as ready for review September 27, 2024 16:44

lgoettgens force-pushed the lg/prefer-symbols branch from fdbbec1 to 6daf07b Compare September 27, 2024 17:52

afkafkafk13 reviewed Sep 28, 2024

View reviewed changes

experimental/BasisLieHighestWeight/src/WeylPolytope.jl Show resolved Hide resolved

lgoettgens force-pushed the lg/prefer-symbols branch from 6daf07b to f3d075e Compare September 30, 2024 08:37

fixes

f3d075e

lgoettgens added the optimization Simpler/more performant code or more/better tests label Sep 30, 2024

lgoettgens requested a review from fingolfin September 30, 2024 10:26

lgoettgens requested a review from lkastner September 30, 2024 10:26

lgoettgens added the triage label Oct 1, 2024

lkastner approved these changes Oct 1, 2024

View reviewed changes

fingolfin approved these changes Oct 6, 2024

View reviewed changes

Update experimental/FTheoryTools/src/TateModels/constructors.jl

08cac81

Co-authored-by: Max Horn <max@quendi.de>

lgoettgens commented Oct 7, 2024

View reviewed changes

experimental/FTheoryTools/src/TateModels/constructors.jl Outdated Show resolved Hide resolved

Update experimental/FTheoryTools/src/TateModels/constructors.jl

61545aa

afkafkafk13 merged commit 1217ab3 into oscar-system:master Oct 7, 2024
28 of 29 checks passed

lgoettgens deleted the lg/prefer-symbols branch October 7, 2024 13:58

lgoettgens removed the triage label Oct 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prefer symbols over strings as indeterminants #4158

Prefer symbols over strings as indeterminants #4158

lgoettgens commented Sep 27, 2024 •

edited

Loading

lgoettgens commented Sep 27, 2024

lgoettgens Sep 27, 2024

fingolfin Oct 6, 2024

benlorenz commented Sep 27, 2024

lgoettgens Sep 27, 2024

benlorenz Sep 27, 2024

afkafkafk13 left a comment •

edited

Loading

codecov bot commented Sep 30, 2024 •

edited

Loading

fingolfin Oct 6, 2024

fingolfin Oct 6, 2024

fingolfin Oct 6, 2024

lgoettgens commented Oct 7, 2024

	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"])

Prefer symbols over strings as indeterminants #4158

Prefer symbols over strings as indeterminants #4158

Conversation

lgoettgens commented Sep 27, 2024 • edited Loading

lgoettgens commented Sep 27, 2024

lgoettgens Sep 27, 2024

Choose a reason for hiding this comment

fingolfin Oct 6, 2024

Choose a reason for hiding this comment

benlorenz commented Sep 27, 2024

lgoettgens Sep 27, 2024

Choose a reason for hiding this comment

benlorenz Sep 27, 2024

Choose a reason for hiding this comment

afkafkafk13 left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Sep 30, 2024 • edited Loading

Codecov Report

fingolfin Oct 6, 2024

Choose a reason for hiding this comment

fingolfin Oct 6, 2024

Choose a reason for hiding this comment

fingolfin Oct 6, 2024

Choose a reason for hiding this comment

lgoettgens commented Oct 7, 2024

lgoettgens commented Sep 27, 2024 •

edited

Loading

afkafkafk13 left a comment •

edited

Loading

codecov bot commented Sep 30, 2024 •

edited

Loading