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.

Sign up for GitHub

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

Jump to bottom

MAINT: lint: enable stacklevel warnings check #19623

Merged
merged 6 commits into from Dec 7, 2023
Merged

MAINT: lint: enable stacklevel warnings check #19623

merged 6 commits into from Dec 7, 2023

Conversation

lucascolley
Copy link
Member

Reference issue

#19604 (comment) @mdhaber

What does this implement/fix?

  • Enable rule B028 which checks that warnings have an explicit stacklevel which is not equal to 1.

Additional information

163 violations right now, will clean them up when I have time.

@lucascolley lucascolley added maintenance Items related to regular maintenance tasks CI Items related to the CI tools such as CircleCI, GitHub Actions or Azure labels Dec 2, 2023
@lucascolley lucascolley added this to the 1.13.0 milestone Dec 2, 2023
lucascolley
lucascolley commented Dec 3, 2023
View reviewed changes
tools/lint.toml Outdated Show resolved Hide resolved
@lucascolley lucascolley marked this pull request as ready for review December 3, 2023 20:10
@lucascolley
Copy link
Member Author

lucascolley commented Dec 3, 2023 •
edited

CI looks relatively happy, how is this @mdhaber @j-bowhay ? I've just put stacklevel=2 everywhere (apart from where there was already a different value, just without the keyword).

@mdhaber
Copy link
Contributor

mdhaber commented Dec 3, 2023 •
edited

That's what I would have suggested. I'll take a quick look in a bit, but overall it can't be worse than the default.

@mdhaber
Copy link
Contributor

mdhaber commented Dec 3, 2023 •
edited

Although maybe private functions should be at least stacklevel=3? Again I'll take a look shortly.

mdhaber
mdhaber reviewed Dec 3, 2023
View reviewed changes
scipy/stats/_continuous_distns.py Outdated
@@ -3492,7 +3492,7 @@ def _argcheck(self, a):
# shape parameter, so warn the user.
message = ('The shape parameter of the erlang distribution '
f'has been given a non-integer value {a!r}.')
warnings.warn(message, RuntimeWarning)
warnings.warn(message, RuntimeWarning, stacklevel=2)
Copy link
Contributor

@mdhaber mdhaber Dec 3, 2023 •
edited

Choose a reason for hiding this comment

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

Yeah I think stacklevel=2 is good for public functions, but since private functions are never called directly by the user, I think stacklevel=3 is a good minimum for private functions. For instance,

from scipy.stats import erlang; erlang.pdf(1, 1.4)

With stacklevel=2:

/Users/matthaberland/Desktop/scipy/scipy/stats/_distn_infrastructure.py:1987: RuntimeWarning: The shape parameter of the erlang distribution has been given a non-integer value array(1.4).
  cond0 = self._argcheck(*args) & (scale > 0)

With stacklevel=3:

<ipython-input-2-f4fb45a9f0ad>:1: RuntimeWarning: The shape parameter of the erlang distribution has been given a non-integer value array(1.4).
  from scipy.stats import erlang; erlang.pdf(1, 1.4)

(which is what we want).

Without going through everything individually, I think that's a good rule.

@j-bowhay
Copy link
Member

j-bowhay commented Dec 3, 2023

Although maybe private functions should be at least stacklevel=3? Again I'll take a look shortly.

Agree private functions should be >2

@lucascolley
Copy link
Member Author

I've had a go at making some changes to stacklevel=3 but I've probably missed a few places or included a few I shouldn't have.

mdhaber
mdhaber reviewed Dec 4, 2023
View reviewed changes
Copy link
Contributor

@mdhaber mdhaber left a comment •
edited

Choose a reason for hiding this comment

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

I didn't make it all the way through, but here are a few cases I found - usually it's when a private method doesn't have a preceding underscore. Please check me on these.

scipy/fft/_fftlog_backend.py Outdated Show resolved Hide resolved
scipy/fft/_fftlog_backend.py Outdated Show resolved Hide resolved
scipy/integrate/_ode.py Outdated Show resolved Hide resolved
scipy/interpolate/_fitpack2.py Outdated Show resolved Hide resolved
scipy/io/_harwell_boeing/hb.py Outdated Show resolved Hide resolved
scipy/optimize/_constraints.py Outdated Show resolved Hide resolved
scipy/optimize/_differentialevolution.py
"solution satisfying the constraints, "
"attempting to polish from the least "
"infeasible solution",
UserWarning, stacklevel=2)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think technically this is correct, even though DE is probably mostly used via differential_evolution. I've wondered what best practice is in such cases. Let's stick with 2 here; just thought I'd mention it.

scipy/optimize/_linesearch.py
@@ -468,7 +469,8 @@ def extra_condition(alpha, phi):
alpha_star = alpha1
phi_star = phi_a1
derphi_star = None
warn('The line search algorithm did not converge', LineSearchWarning)
warn('The line search algorithm did not converge',
LineSearchWarning, stacklevel=2)
Copy link
Contributor

Choose a reason for hiding this comment

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

GH shows this like:

image

But shouldn't it be able to tell that that's not the relevant function? Do you think this is a bug?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep definitely a bug, I noticed this in quite a few places. Might be worth browsing upstream but I'd imagine they are aware.

scipy/optimize/_lsq/least_squares.py Outdated Show resolved Hide resolved
scipy/optimize/_shgo.py Outdated Show resolved Hide resolved
mdhaber
mdhaber reviewed Dec 4, 2023
View reviewed changes
Copy link
Contributor

@mdhaber mdhaber left a comment

Choose a reason for hiding this comment

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

This is the only other thing I found. After these changes are made, I think we can merge.

scipy/optimize/_trustregion_constr/projections.py Outdated Show resolved Hide resolved
@lucascolley @mdhaber
MAINT: stacklevel adjustments from review
e49af23 
Co-authored-by: Matt Haberland <mhaberla@calpoly.edu>
@lucascolley
Copy link
Member Author

thanks for the thorough review @mdhaber ! I've pushed in all of your suggested adjustments.

j-bowhay
j-bowhay reviewed Dec 7, 2023
View reviewed changes
dev.py Outdated Show resolved Hide resolved
@mdhaber @j-bowhay
Update dev.py
2f1469a 
[skip ci]

Co-authored-by: Jake Bowhay <60778417+j-bowhay@users.noreply.github.com>
@mdhaber mdhaber merged commit 8166222 into scipy:main Dec 7, 2023
@lucascolley lucascolley deleted the lint-b028 branch December 7, 2023 17:06
@lucascolley lucascolley modified the milestones: 1.13.0, 1.12.0 Dec 7, 2023
This was referenced Dec 8, 2023
Merged
Merged
@j-bowhay j-bowhay mentioned this pull request Apr 20, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdhaber mdhaber mdhaber left review comments

@j-bowhay j-bowhay j-bowhay left review comments

Assignees
No one assigned
Labels
CI Items related to the CI tools such as CircleCI, GitHub Actions or Azure maintenance Items related to regular maintenance tasks
Projects
None yet
Milestone
1.12.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@lucascolley @mdhaber @j-bowhay