Skip to content

Conversation

@kandersolar
Copy link
Member

@kandersolar kandersolar commented Apr 29, 2022

  • Code changes are covered by tests
  • Code changes have been evaluated for compatibility/integration with TrendAnalysis
  • New functions added to __init__.py
  • API.rst is up to date, along with other sphinx docs pages
  • Example notebooks are rerun and differences in results scrutinized
  • Updated changelog

This is a change I've wanted to make for a while: specify a better stacklevel when emitting warnings. The stacklevel controls which line of code is shown as causing the warning, with the default (stacklevel=1) being the call to warnings.warn itself. Increasing the stacklevel usually results in a more useful line of code being shown. For example, our current example notebooks show warnings like this:

/Users/mdecegli/opt/anaconda3/envs/xgboost/lib/python3.7/site-packages/rdtools/soiling.py:15: UserWarning: The soiling module is currently experimental. The API, results, and default behaviors may change in future releases (including MINOR and PATCH releases) as the code matures.
  'The soiling module is currently experimental. The API, results, '

The source code line shown below the warning is not really useful to the user. In this case it's just the string literal containing the warning message; other times it's something like warnings.warn(.... What would be more useful is to show one level up in the call stack (stacklevel=2), which would look like this:

c:\users\kanderso\software\anaconda3\envs\rdtools-dev\lib\site-packages\rdtools\analysis_chains.py:582: UserWarning: The soiling module is currently experimental. The API, results, and default behaviors may change in future releases (including MINOR and PATCH releases) as the code matures.
  from rdtools import soiling

The best improvements are when the relevant higher-level call is a single line of code. Multi-line expressions are not always handled very well, for example:

c:\users\kanderso\software\anaconda3\envs\rdtools-dev\lib\site-packages\rdtools\analysis_chains.py:850: UserWarning: The soiling module is currently experimental. The API, results, and default behaviors may change in future releases (including MINOR and PATCH releases) as the code matures.
  results_dict['calc_info'], **kwargs)

Another complication is that the relevant stack level might be different depending how the function is called. For example logic_clip_filter can be called directly (stacklevel=2) or indirectly via clip_filter(..., model='logic') (stacklevel=3).

Regardless, specifying stacklevel doesn't make things any worse, so I don't see a reason not to use them. This IPython blog post has a bit of interesting commentary on this topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants