-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
fix(python): dont show wrong inefficient apply warning when taking numpy func of constant #10103
Conversation
3d09d5e
to
4052105
Compare
block_offsets = list(expression_blocks.keys()) | ||
block_offsets.remove(start) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -113,6 +113,13 @@ | |||
("c", lambda x: json.loads(x), 'pl.col("c").str.json_extract()'), | |||
] | |||
|
|||
NOOP_TEST_CASES = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moving these here from test_inefficient_apply
, we should probably be testing the "no-op" cases across all supported python versions too
Hmm, turns out it only takes two/three lines to support this properly; as the result will resolve to a const it can be integrated more or less 'as is' - see what you think? 😅 Commit: f44b1ed. Tried it out with (Unrelated change: removed |
well you're a bit good, aren't you? 🙌 |
closing in favour of #10104 |
I think you mean "a bit OCD" ... 🤣 |
I'd missed this when reviewing, but a "nonsense" warning is currently shown in some cases:
shows
I think we need to be extra-defensive here, and match as explicitly as we can - I've added
param_name_index
to the function so we can check that parameter name is exactly in the position we expect it to bewith this PR, the code above wouldn't display any warning, as
np.sin(3)
isn't a simple-enough constant to be considered (yet!)