Skip to content

[refurb] Implement redundant-log-base (FURB163)#8842

Merged
charliermarsh merged 6 commits intoastral-sh:mainfrom
tjkuson:redundant-log-base
Nov 27, 2023
Merged

[refurb] Implement redundant-log-base (FURB163)#8842
charliermarsh merged 6 commits intoastral-sh:mainfrom
tjkuson:redundant-log-base

Conversation

@tjkuson
Copy link
Copy Markdown
Contributor

@tjkuson tjkuson commented Nov 26, 2023

Summary

Implement simplify-math-log as redundant-log-base (FURB163).

Auto-fixes

import math

math.log(2, 2)

to

import math

math.log2(2)

Related to #1348.

Test Plan

cargo test

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Nov 26, 2023

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@tjkuson tjkuson marked this pull request as ready for review November 26, 2023 19:36
Copy link
Copy Markdown
Member

@zanieb zanieb left a comment

Choose a reason for hiding this comment

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

Nice! This lgtm except the rule needs to go through preview.

Co-authored-by: Zanie Blue <contact@zanie.dev>
@bluetech
Copy link
Copy Markdown
Contributor

bluetech commented Nov 27, 2023

IMO the term "redundant" is not quite right (except the math.log(x, math.e) case). The reason to prefer math.log2(x) over math.log(x, 2) is floating-point accuracy, as mentioned in the function docs.

@tjkuson
Copy link
Copy Markdown
Contributor Author

tjkuson commented Nov 27, 2023

Yeah, that's fair. I did think about which name to use when porting the rule, but never found one I fully liked (it has to both describe both the math.log(foo, math.e) case and the math.log(foo, 2) case). I settled for redundant log base to mean 'there is a way of writing this logarithmic function without specifying a base' instead of 'just remove the base argument'.

I also tried to make the rule message be explicit with what its recommendation is, to guard against confusion.

FURB163.py:15:1: FURB163 [*] Replace with `math.log2(1)`

Happy for name to changed, though. I'm not a huge fan of it.

Also, for what it's worth, the corresponding refurb rule simplify-math-log cites readability, not performance, as the motivation. I cited both readability and precision when porting the rule in the documentation (as well as efficiency; I found the fixes ran faster).

EDIT: Maybe superfluous-log-base?

EDIT 2: 'Accuracy', not 'precision', is the correct term to use here.

@zanieb
Copy link
Copy Markdown
Member

zanieb commented Nov 27, 2023

I think the name is fine. You could use imprecise-log-base if you really wanted but I imagine for most users the floating point accuracy is not going to be critical.

@bluetech
Copy link
Copy Markdown
Contributor

Got it on the name. So my only nit is: in the rule description, replace "precise" with "accurate"; both math.log and math.log2 have the same FP precision (double), but the latter is more accurate (for base 2).

@charliermarsh charliermarsh enabled auto-merge (squash) November 27, 2023 23:44
@charliermarsh charliermarsh added rule Implementing or modifying a lint rule preview Related to preview mode features labels Nov 27, 2023
@charliermarsh charliermarsh merged commit 60eb11f into astral-sh:main Nov 27, 2023
@tjkuson tjkuson deleted the redundant-log-base branch November 28, 2023 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

preview Related to preview mode features rule Implementing or modifying a lint rule

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants