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

Avoid the use of stride tricks to make code more readable #89

Merged
merged 1 commit into from
Dec 25, 2024

Conversation

hmaarrfk
Copy link
Contributor

@hmaarrfk hmaarrfk commented Jul 7, 2024

Admittedly this is a style issue, but I feel like stridetricks isn't very useful in this particular case and it somewhat destroys readability for me.

While the reshaping is cute, I honestly don't think it helps much with the python overhead.

I hit this code in trying to implment frustum culling for my application (though I still haven't succeeded in getting a performance improvement).

Let me know what you think.

I think there is also some complexity in the code due to an attempt at making it generaizable to beyond a single "aabb" object.

However, I didn't see any tests for this, and so my presumption is that this is problem and likely the cause of much of the original code compelxity.

@hmaarrfk hmaarrfk requested a review from Korijn as a code owner July 7, 2024 19:00
@almarklein
Copy link
Member

I think there is also some complexity in the code due to an attempt at making it generaizable to beyond a single "aabb" object.

Yes, (almost) all functions in pylinalg are meant to also work on batches (i.e. arrays) of objects. That ability provides a big performance benefit in cases where multiple objects must be transformed. If the test for that is missing for this function, we should probably add that too, to make sure the changes don't break that ability.

@Korijn
Copy link
Contributor

Korijn commented Dec 25, 2024

We can merge this if you just run ruff on the changes.

@hmaarrfk
Copy link
Contributor Author

??? locally it tells me there is nothing to change:

(dev) ✔ ~/git/pylinalg [simplify_aabb|✔] 
mark@carbon $ ruff --version
ruff 0.8.4
(dev) ✔ ~/git/pylinalg [simplify_aabb|✔] 
mark@carbon $ ruff check --verbose pylinalg/misc.py 
[2024-12-25][08:55:51][ruff::resolve][DEBUG] Using configuration file (via parent) at: /home/mark/git/pylinalg/pyproject.toml
[2024-12-25][08:55:51][ruff_linter::settings::types][DEBUG] Detected minimum supported `requires-python` version: 3.9
[2024-12-25][08:55:51][ignore::gitignore][DEBUG] opened gitignore file: /home/mark/.cvsignore
[2024-12-25][08:55:51][ruff::commands::check][DEBUG] Identified files to lint in: 3.464185ms
[2024-12-25][08:55:51][ruff::commands::check][DEBUG] Checked 1 files in: 18.243µs
All checks passed!
(dev) ✔ ~/git/pylinalg [simplify_aabb|✔] 
mark@carbon $

@Korijn
Copy link
Contributor

Korijn commented Dec 25, 2024

Git pull my commits and reinstall your full venv. I redid the whole packaging setup.

(Merry Christmas!)

@hmaarrfk
Copy link
Contributor Author

hmaarrfk commented Dec 25, 2024

$ conda create --name linalg python=3.12
$ conda activate linalg
$ pip install -e ".[dev,docs,examples]"
$ ruff check .
All checks passed!
$ which ruff
/home/mark/miniforge3/envs/linagl/bin/ruff

still nothing.

Merry Christmas! You know making the most of the time off ;)

@Korijn
Copy link
Contributor

Korijn commented Dec 25, 2024

It's ruff format . that you're missing :)

Admittedly this is a style issue, but I feel like stridetricks isn't
very useful in this particular case and it somewhat destroys readability
for me.

While the reshaping is cute, I honestly don't think it helps much with
the python overhead.

I hit this code in trying to implment frustum culling for my application
(though I still haven't succeeded in getting a performance improvement).

Let me know what you think.

I think there is also some complexity in the code due to an attempt at
making it generaizable to beyond a single "aabb" object.

However, I didn't see any tests for this, and so my presumption is that
this is problem and likely the cause of much of the original code
compelxity.
@hmaarrfk
Copy link
Contributor Author

1. -> 1.0

took so much engineering time ^_^

@hmaarrfk
Copy link
Contributor Author

maybe one day i'll be brave enough to use ruff format in my own projects!

Copy link
Contributor

@Korijn Korijn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the shiney new future man

@Korijn Korijn merged commit 4015b5c into pygfx:main Dec 25, 2024
10 checks passed
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.

3 participants