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.

Sign up for GitHub

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

Jump to bottom

WIP implement #211: native min and max over arrays via glm::min / glm::max #213

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

WIP implement #211: native min and max over arrays via glm::min / glm::max #213

wants to merge 5 commits into from

Conversation

cspotcode
Copy link
Contributor

@cspotcode cspotcode commented Feb 26, 2023 •
edited
Loading

Fixes #211

I think I found where this needs to be added in the code, but it would be way faster for you to point out what exactly I should be copying to generate the correct switch-case block to call all the templated implementations of apply_min. I see that apply_min takes a std::vec and I'm pretty sure that glmArray is not a std::vec. So does this need a new apply_min_of_glmArray template?

@cspotcode cspotcode marked this pull request as draft February 26, 2023 19:52
@Zuzu-Typ
Copy link
Owner

Unfortunately, this is a bit more complex than it might seem at first.

If the array has elements of a number type (e.g. 32-bit floating point values), it's trivial to perform a min or max operation on them. But what if the elements are vectors, quaternions or matrices?
Since you can't define a logical "less than" operation for vectors (for example is vec2(1, 0) < vec2(0, 1) or vec2(0, 1) < vec2(1, 0)?), you would have to follow glm's way, which is calculating the minimum value of each component of the vector. This however isn't exactly trivial to implement.
I'm very happy for your support and that you're trying to find a solution for the issue, but I'd rather save you the headache of trying to understand the enormous amount of code I have assembled over the years (which might be required to implement the min and max functions).

@cspotcode
Copy link
Contributor Author

you would have to follow glm's way, which is calculating the minimum value of each component of the vector. This however isn't exactly trivial to implement.

I was imagining that, if PyGLM already has an implementation of min(vec2, vec2) then I would piggy-back on that implementation. Is that a bad approach?

@Zuzu-Typ
Copy link
Owner

Zuzu-Typ commented Feb 27, 2023 •
edited
Loading

I was imagining that, if PyGLM already has an implementation of min(vec2, vec2) then I would piggy-back on that implementation. Is that a bad approach?

That would be a good idea in theory, but quite inefficient. You'd have to call the function for every element (except for the first) in the array. E.g. for an array with 5 components: min(min(min(min(e1, e2), e3), e4), e5)

A better approach would be to split the array into components if it is a vec, quat or mat array, call the min function with each one of those components and re-assemble the result array. I have some ideas to make this a little more performance efficient (e.g. by using multithreading)

Well, actually I was wrong. It is probably the most efficient way. That is, using glm::min. You wouldn't want to use the apply_min or min_ functions provided by PyGLM though, because it would be a significant overhead.

@cspotcode
Copy link
Contributor Author

Ok thanks, I see that the code has switch-case blocks where you check the type of something, then call the correct templated implementation. I think I need to do the same here but call a templated implementation of glm::min. Is that correct?

PyGLM/PyGLM/functions/detail/func_common.h

Lines 239 to 263 in 9f2cdfe

#define min_GEN_TYPE_SWITCH_STATEMENT_FOR_VECTOR(L) \
switch (pti->format) { \
case get_format_specifier<float>(): \
return apply_min_from_PyObject_vector_vector<L, float>(items); \
case get_format_specifier<double>(): \
return apply_min_from_PyObject_vector_vector<L, double>(items); \
case get_format_specifier<int32>(): \
return apply_min_from_PyObject_vector_vector<L, int32>(items); \
case get_format_specifier<uint32>(): \
return apply_min_from_PyObject_vector_vector<L, uint32>(items); \
case get_format_specifier<int64>(): \
return apply_min_from_PyObject_vector_vector<L, int64>(items); \
case get_format_specifier<uint64>(): \
return apply_min_from_PyObject_vector_vector<L, uint64>(items); \
case get_format_specifier<int16>(): \
return apply_min_from_PyObject_vector_vector<L, int16>(items); \
case get_format_specifier<uint16>(): \
return apply_min_from_PyObject_vector_vector<L, uint16>(items); \
case get_format_specifier<int8>(): \
return apply_min_from_PyObject_vector_vector<L, int8>(items); \
case get_format_specifier<uint8>(): \
return apply_min_from_PyObject_vector_vector<L, uint8>(items); \
case get_format_specifier<bool>(): \
return apply_min_from_PyObject_vector_vector<L, bool>(items); \
}

What fields should I be checking on the array to switch-case on? Which fields on glmArray indicate the shape of the contents, and is there an example of a reinterpret_cast on glmArray.data that will let me iterate it as a C array of vec2?

PyGLM/PyGLM/types/types.h

Lines 291 to 303 in 9f2cdfe

struct glmArray {
PyObject_HEAD
char format;
uint8 shape[2];
uint8 glmType;
Py_ssize_t nBytes;
Py_ssize_t itemCount;
Py_ssize_t dtSize;
Py_ssize_t itemSize;
PyTypeObject* subtype;
PyObject* reference;
bool readonly;
void* data;

Does glmArray.shape store the dimensions of the contents, so shape = 2,1 means it stores vec2s? To check the type of the numbers -- float, int, etc -- should I be looking at glmType or format or subtype?

@cspotcode
Copy link
Contributor Author 

❯ hyperfine --show-output --runs 5 --parameter-list impl py,glm 'python ./test_min_{impl}.py'
Benchmark #1: python ./test_min_py.py
  Time (mean ± σ):     10.412 s ±  0.158 s    [User: 10.408 s, System: 0.004 s]
  Range (min … max):   10.169 s … 10.590 s    5 runs
 
Benchmark #2: python ./test_min_glm.py
  Time (mean ± σ):     880.9 ms ±  22.3 ms    [User: 876.6 ms, System: 4.0 ms]
  Range (min … max):   854.0 ms … 900.1 ms    5 runs
 
Summary
  'python ./test_min_glm.py' ran
   11.82 ± 0.35 times faster than 'python ./test_min_py.py'

@cspotcode cspotcode changed the title Fix #211: broken attempt at this; need guidance WIP implement #211: native min and max over arrays via glm::min / glm::max Mar 2, 2023
@cspotcode
Copy link
Contributor Author

This'll need a lot more macro work to support all possible data types that you can store in a pyglm array, but I have it prototyped for arrays of floats and vec2s. Benchmark shows it's definitely much faster.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

glm.min and glm.max benchmark slower than python 3.11 min and max operating over glm.array[glm.c_float]
2 participants
@cspotcode @Zuzu-Typ