Catch only ParameterNotUsed warnings in _catch_warnings
#2753
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2753 +/- ##
=======================================
+ Coverage 99.9% 99.9% +0.1%
=======================================
Files 300 300
Lines 27705 27716 +11
=======================================
+ Hits 27656 27667 +11
Misses 49 49
Continue to review full report at Codecov.
|
eccabay
requested review from
dsherry,
chukarsten,
freddyaboulton,
angela97lin,
bchen1116,
ParthivNaresh and
christopherbunn
and removed request for
dsherry and
chukarsten
| def test_pipeline_parameter_warnings_only_parameternotused( | ||
| mock_data_check, X_y_regression | ||
| ): | ||
| mock_data_check.side_effect = UserWarning("Big bad warning") |
| else: | ||
| final_message = final_message.intersection(msg.message.components) | ||
| warnings.warn(ParameterNotUsedWarning(final_message)) | ||
| if isinstance(msg.message, ParameterNotUsedWarning): |
There was a problem hiding this comment.
There might be a bug here? What if the warning list is [RandomWarning(...), ParameterNotUsedWarning(...)].
Then idx will be 1 when the isinstance check is true so we'll do the intersection of an empty set and the ParameterNotUsedWarning, so we'll wipe out the warning we want to log?
There was a problem hiding this comment.
Oh shoot, I just merged this but you're totally right 🤦♀️
It should be a pretty simple fix to replace the idx check with checking if the set is empty or not instead - I'll put up a PR for that shortly!
There was a problem hiding this comment.
Actually, there is a bug here, but it's not that! The check of if len(warning_list) == len(self.allowed_pipelines) and len(warning_list) > 0: would prevent the warning list of [RandomWarning(...), ParameterNotUsedWarning(...)] from ever reaching that point in the code, since in that case len(warning_list != len(self.allowed_pipelines).
So in this case we would indeed fail to log the ParameterNotUsedWarning, but not for the reason highlighted.
Also, this highlights another issue where we currently catch and then do not raise all non-ParameterNotUsedWarning warnings during make_pipeline. Some sleuthing found that PR #2564 added the warning catches where they were not before, so this behavior was unwanted but introduced then. I'm going to file an issue and put up a PR for raising all warnings here again, since the size of this change just grew.
Closes #2652