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.

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

chore(python): clean up bytecode parsing a bit #13221

Merged
merged 2 commits into from
Dec 27, 2023

Conversation

MarcoGorelli
Copy link
Collaborator

@MarcoGorelli MarcoGorelli commented Dec 23, 2023

There's 3 cleanups I'm suggesting here:

  1. make try blocks smaller. I think this is uncontroversial

  2. remove an unused try block in _is_raw_function. The whole test suite passes without it, I think it the code can be simplified by just removing it

  3. removing the _can_attempt_rewrite dict. I think this was originally introduced for performance reasons, but:

    • I'm not seeing any perf difference here vs on main (timing pl.col('a').map_elements(lambda x: abs(x)) here and on main takes about 8.988900013459247e-05 seconds on my laptop
    • it adds quite a lot of complexity

    It's already super-fast, I don't think the extra complexity is warranted

@github-actions github-actions bot added internal An internal refactor or improvement python Related to Python Polars labels Dec 23, 2023
@MarcoGorelli MarcoGorelli marked this pull request as ready for review December 23, 2023 13:42
@MarcoGorelli
Copy link
Collaborator Author

@alexander-beedie fancy taking a look?

@alexander-beedie
Copy link
Collaborator

@alexander-beedie fancy taking a look?

Definitely! Can dig in tonight (or tomorrow morning) and see if I can recall the primary scenario this speeds up (or used to speed up 😂)

@alexander-beedie alexander-beedie merged commit 4304ffe into pola-rs:main Dec 27, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal An internal refactor or improvement python Related to Python Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants