Prefer symbols over strings in polynomial_ring

[lgoettgens]

See Nemocas/AbstractAlgebra.jl#1804

[lkastner]

Please add a test to ensure this is not done wrong in src again. We don't want to repeat this procedure over and over. Also add a style guide entry that symbols should be used instead of strings.

[codecov]

Codecov Report

Attention: Patch coverage is 66.15385% with 22 lines in your changes missing coverage. Please review.

Project coverage is 84.65%. Comparing base (55ea957) to head (5d631fb).
Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
experimental/ModStd/src/ModStdNF.jl	10.00%	9 Missing ⚠️
...binatorics/Matroids/matroid_strata_grassmannian.jl	0.00%	2 Missing ⚠️
experimental/GroebnerWalk/benchmark/agk.jl	0.00%	1 Missing ⚠️
experimental/GroebnerWalk/src/special-ideals.jl	0.00%	1 Missing ⚠️
experimental/IntersectionTheory/src/Main.jl	83.33%	1 Missing ⚠️
experimental/LinearQuotients/src/cox_rings.jl	0.00%	1 Missing ⚠️
.../QuadFormAndIsom/src/hermitian_miranda_morrison.jl	0.00%	1 Missing ⚠️
...ntersections/src/homogeneous_polynomial_actions.jl	50.00%	1 Missing ⚠️
...metricIntersections/src/symmetric_grassmannians.jl	66.66%	1 Missing ⚠️
src/Groups/matrices/stuff_field_gen.jl	50.00%	1 Missing ⚠️
... and 3 more

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4133   +/-   ##
=======================================
  Coverage   84.65%   84.65%           
=======================================
  Files         626      626           
  Lines       84313    84313           
=======================================
  Hits        71377    71377           
  Misses      12936    12936           

Files with missing lines	Coverage Δ
experimental/DoubleAndHyperComplexes/test/ext.jl	100.00% <ø> (ø)
...ntal/FTheoryTools/src/FamilyOfSpaces/attributes.jl	100.00% <ø> (ø)
...al/FTheoryTools/src/FamilyOfSpaces/constructors.jl	100.00% <ø> (ø)
...TheoryTools/src/HypersurfaceModels/constructors.jl	96.72% <ø> (ø)
...rimental/FTheoryTools/src/TateModels/attributes.jl	86.58% <ø> (ø)
...mental/FTheoryTools/src/TateModels/constructors.jl	97.46% <ø> (ø)
...FTheoryTools/src/WeierstrassModels/constructors.jl	95.65% <ø> (ø)
experimental/FTheoryTools/src/auxiliary.jl	76.00% <ø> (ø)
...imental/FTheoryTools/src/standard_constructions.jl	100.00% <100.00%> (ø)
...erimental/FTheoryTools/test/hypersurface_models.jl	100.00% <ø> (ø)
... and 112 more

[fingolfin]

Please add a test to ensure this is not done wrong in src again.

Not sure what you mean here? Using strings is not "wrong". Adding a test that reliably prevents new code from using this would be super hard to do, I don't think it is reasonable to expect this from this PR.

We don't want to repeat this procedure over and over. Also add a style guide entry that symbols should be used instead of strings.

I agree a style guide entry suggesting (and explaining!) that would be a good idea. But again I don't think it is necessary to do it in this PR. Of course if @lgoettgens has the time for it, great. But otherwise either a follow-up PR, or an issue reminding us to do this should be fine?

[lgoettgens]

I can add a style guide entry, but would prefer to do that in a follow-up PR. It would be great to get this in ASAP before we get any conflicts.

…

On September 23, 2024 2:32:58 PM GMT+02:00, Lars Kastner ***@***.***> wrote: Please add a test to ensure this is not done wrong in `src` again. We don't want to repeat this procedure over and over. Also add a style guide entry that symbols should be used instead of strings. -- Reply to this email directly or view it on GitHub: #4133 (comment) You are receiving this because you authored the thread. Message ID: ***@***.***>

[fingolfin]

This is huge, so I did not look at every changed line. But I looked at many samples across varies files and they all seemed reasonable. So if the tests pass, we should just merge it.

[lkastner]

Not sure what you mean here? Using strings is not "wrong". Adding a test that reliably prevents new code from using this would be super hard to do, I don't think it is reasonable to expect this from this PR.

It was just a jab at this PR being super large without significantly improving performance. As you said in Nemocas/AbstractAlgebra.jl#1804:

Of course code where this noticeable would be code that calls polynomial_ring
far too many times, and should be changed to not do that.

So since we are approaching this bureaucratically I commented bureaucratically. ;)