DM-22727: Add numpy warnings catch to DiaCalculationPlugins #70
Conversation
There was a problem hiding this comment.
This looks fine as-is, but I think this would be a good use for a decorator, e.g. @catchNumpyWarnings. If you set it up so you can pass strings of the warnings to ignore to it, then you could simply add the decorator to each function instead of duplicating the with np.warnings.catch_warnings(): block everywhere.
| @@ -559,6 +573,7 @@ def calculate(self, | |||
| **kwargs | |||
| Any additional keyword arguments that may be passed to the plugin. | |||
| """ | |||
|
|
|||
There was a problem hiding this comment.
No blank line after a function docstring.
| return pd.Series(dict((tileName, pTile) | ||
| for tileName, pTile in zip(pTileNames, pTiles))) | ||
| with np.warnings.catch_warnings(): | ||
| np.warnings.filterwarnings('ignore', r'All-NaN slice encountered') |
There was a problem hiding this comment.
Can we please use double quotes to be consistent with the rest of the file? Also, why is this a raw string?
There was a problem hiding this comment.
Done. The raw string is based on what I saw in examples both online and in our own code: https://github.com/lsst/pipe_tasks/blob/448f008be835c65bd1e4254d7fddb65aacc18f68/python/lsst/pipe/tasks/functors.py#L541
There was a problem hiding this comment.
Odd. It can't be doing anything useful can it? Sounds like cargo cult 😄
|
Hey, @isullivan can you take a look at the new commit and see if that works for you? |
|
With apologies for the drive-by comment —
|
| def decoratorCatchWarnings(func): | ||
| @functools.wraps(func) | ||
| def wrapperCatchWarnings(*args, **kwargs): | ||
| np.warnings.catch_warnings() |
There was a problem hiding this comment.
Make this with np.warnings.catch_warnings(): and indent the for loop and return statement below.
There was a problem hiding this comment.
Thanks for catching this. Definitely a bug I introduced while re-writing this.
|
As written, this swallows the specified warnings globally, so you definitely want to still use |
|
Verified the changes are working on the ap_verify CI dataset. Will submit another Jenkins just in case. Mind having one last look @isullivan? |
|
My linter still complains about the blank line on 589, which you should remove. Otherwise, it looks good. |
Fix linting. Create decorator for cathing numpy warnings. move to python warnings. Fix context bug. Remove blank line
morriscb
force-pushed
the
tickets/DM-22727
branch
from
1cb847a
to
d66c66d
Compare
No description provided.