Skip to content
New issue

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

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

Already on GitHub? # to your account

Prefer symbols for VarNames over strings #1804

Merged
merged 3 commits into from
Sep 23, 2024
Merged

Conversation

fingolfin
Copy link
Member

Both are valid and allowed and will remain so. But using symbols overall
avoids a bunch of allocations, so it makes sense to adjust our internal
code as well as user-facing documentation to use symbols to set a good
precedent.

To demonstrate the effect, here a benchmark with strings:

julia> @btime polynomial_ring(QQ, ["x", "y"]);
  756.274 ns (27 allocations: 1.22 KiB)

Here the same with symbols:

julia> @btime polynomial_ring(QQ, [:x, :y]);
  535.963 ns (20 allocations: 896 bytes)

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.

Both are valid and allowed and will remain so. But using symbols overall
avoids a bunch of allocations, so it makes sense to adjust our internal
code as well as user-facing documentation to use symbols to set a good
precedent.

To demonstrate the effect, here a benchmark with strings:

    julia> @Btime polynomial_ring(QQ, ["x", "y"]);
      756.274 ns (27 allocations: 1.22 KiB)

Here the same with symbols:

    julia> @Btime polynomial_ring(QQ, [:x, :y]);
      535.963 ns (20 allocations: 896 bytes)

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*.
Copy link

codecov bot commented Sep 20, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.02%. Comparing base (fd3af98) to head (5d22a95).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/Rings.jl 0.00% 1 Missing ⚠️
src/algorithms/MPolyFactor.jl 66.66% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1804   +/-   ##
=======================================
  Coverage   88.02%   88.02%           
=======================================
  Files         119      119           
  Lines       30057    30057           
=======================================
+ Hits        26457    26459    +2     
+ Misses       3600     3598    -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@lgoettgens lgoettgens left a comment

Choose a reason for hiding this comment

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

Looks good, but there are doctest failures

@lgoettgens lgoettgens enabled auto-merge (squash) September 23, 2024 09:50
@lgoettgens lgoettgens merged commit 548894f into master Sep 23, 2024
29 of 30 checks passed
@lgoettgens lgoettgens deleted the mh/prefer-varname-symbols branch September 23, 2024 11:10
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants