Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove redundant _parse_symbol methods #4535

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

fingolfin
Copy link
Member

Also replace some uses of gens by symbols, and exploit that graded_polynomial_ring can take multiple symbol lists to simplify some of the code.

@@ -255,7 +249,7 @@ function blow_up_points(X::AbstractVariety, n::Int; symbol::String = "e")
if n == 1
symbs = [SED]
else
symbs = _parse_symbol(SED, 1:n)
symbs = ["$SED[$i]" for i in 1:n]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is still a _parse_symbol method for this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, though it is now only is used in a single file, Main.jl.

Copy link

codecov bot commented Feb 4, 2025

Codecov Report

Attention: Patch coverage is 97.29730% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.42%. Comparing base (dac55d7) to head (1037bcb).
Report is 24 commits behind head on master.

Files with missing lines Patch % Lines
experimental/IntersectionTheory/src/blowup.jl 80.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4535   +/-   ##
=======================================
  Coverage   84.42%   84.42%           
=======================================
  Files         672      672           
  Lines       89164    89158    -6     
=======================================
- Hits        75273    75269    -4     
+ Misses      13891    13889    -2     
Files with missing lines Coverage Δ
experimental/IntersectionTheory/src/Bott.jl 74.01% <100.00%> (ø)
experimental/IntersectionTheory/src/Main.jl 93.29% <100.00%> (+0.02%) ⬆️
experimental/IntersectionTheory/src/Misc.jl 96.87% <ø> (+10.76%) ⬆️
experimental/IntersectionTheory/src/blowup.jl 98.79% <80.00%> (-0.06%) ⬇️

... and 1 file with indirect coverage changes

@fingolfin fingolfin force-pushed the mh/_parse_symbol branch 2 times, most recently from c83c0c7 to 40eb5d2 Compare February 4, 2025 15:08
@@ -2523,7 +2520,7 @@ function abs_flag(dims::Vector{Int}; base::Ring=QQ, symbol::String="c")
syms = vcat([_parse_symbol(symbol, i, 1:r) for (i,r) in enumerate(ranks)]...)
# FIXME ordering
# ord = prod(ordering_dp(r) for r in ranks)
R = graded_polynomial_ring(base, syms, vcat([collect(1:r) for r in ranks]...))[1]
R = graded_polynomial_ring(base, syms, vcat([1:r for r in ranks]...))[1]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
R = graded_polynomial_ring(base, syms, vcat([1:r for r in ranks]...))[1]
R = graded_polynomial_ring(base, syms, reduce(vcat, (1:r for r in ranks)))[1]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine by me, but the pattern occurs several times in this file, so perhaps they should be changed all at once.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to say:

julia> ranks = [3,1,5];

julia> @b vcat([1:r for r in $ranks]...)
92.871 ns (7 allocs: 336 bytes)

julia> @b reduce(vcat, (1:r for r in $ranks))
58.886 ns (7 allocs: 320 bytes)

julia> @b reduce(vcat, [1:r for r in $ranks])
48.961 ns (6 allocs: 320 bytes)

julia> @b reduce(vcat, 1:r for r in $ranks)
60.154 ns (7 allocs: 320 bytes)

So the version which does reduce over a vector is best here. None of these is great -- it could be done with a single allocation:

function parse_ranks(ranks)
  res = Vector{Int}(undef, sum(ranks))
  i = 1
  for r in ranks
    for j in 1:r
      res[i] = j
      i += 1
    end
  end
  return res
end

function parse_ranks2(ranks)
  res = sizehint!(Int[], sum(ranks))
  for r in ranks
    append!(res, 1:r)
  end
  return res
end

gives

julia> @b parse_ranks($ranks)
20.239 ns (2 allocs: 128 bytes)

julia> @b parse_ranks2($ranks)
36.541 ns (2 allocs: 128 bytes)

Of course none of this really matters, you just nerd-sniped me successfully ;-).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The splatting version has the additional overhead of compiling a new method specialization every time you call it with a different number of arguments. This probably won't be caught by your chairmark

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but I am not using the splatting version, I am using reduce with a Vector which was best in the micro benchmarks.

Also replace some uses of `gens` by `symbols`
@fingolfin fingolfin added the release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes label Feb 6, 2025
@fingolfin
Copy link
Member Author

OK to merge this? Or any concerns?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants