Skip to content

Add determinant, laplace and cross products to math_vector.c3 #2040

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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

jopadan
Copy link

@jopadan jopadan commented Mar 11, 2025

@lerno
Copy link
Collaborator

lerno commented Mar 12, 2025

Can you please add some actual tests, look into test/unit how to construct them. You can then use c3c compile-test <path to folder> to test.

@jopadan jopadan force-pushed the master branch 3 times, most recently from 9644ecd to 69a9c9d Compare March 12, 2025 18:34
@jopadan jopadan changed the title Add cross products Add determinant, laplace and cross products to vector_math.c3 Mar 12, 2025
@jopadan jopadan changed the title Add determinant, laplace and cross products to vector_math.c3 Add determinant, laplace and cross products to math_vector.c3 Mar 12, 2025
@jopadan jopadan force-pushed the master branch 2 times, most recently from cfe8d35 to e6158c3 Compare March 12, 2025 19:06
@lerno
Copy link
Collaborator

lerno commented Mar 13, 2025

Sorry, that part wasn't supposed to be there. Not that it harms anything but..

@lerno
Copy link
Collaborator

lerno commented Mar 13, 2025

One question I have is whether the vector operations are the most efficient this way (I'm thinking of the 4 cross product) or not, have you measured the resulting output compared to writing it out on -O1? Also, possibly the two different windings should be two different functions instead.

@lerno
Copy link
Collaborator

lerno commented Mar 20, 2025

I am confused. There is this: macro float[<*>].determinant2(self, float[<*>] b) => determinant2(self,b);

But this looks like it's assuming

| self.x self.y | 
| b.x       b.y  |

Which means it's not applicable to any float vectors that aren't len = 2. Also, it is only defined for floats, as opposed to both doubles and floats.

I am also a little concerned that only some of them have tests.

@jopadan
Copy link
Author

jopadan commented Mar 23, 2025

I don't see why the implementation should not work for vectors that aren't len = 2.
It supports len = n vectors with only taking m elements specified by determinant[m].

@jopadan
Copy link
Author

jopadan commented Mar 23, 2025

See updated implementation for double

@jopadan
Copy link
Author

jopadan commented Mar 23, 2025

Is your 2x2 post concern about column/row major format?

@lerno
Copy link
Collaborator

lerno commented Mar 24, 2025

I think it's fairly confusing to have this. If you really want the example taking a subset of the vector then that isn't hard. Like for the determinant case for a 4 vector and you want to do the 2x2 determinant, why not simply do it explicitly? x.xy.det(y.xy)

@lerno lerno added this to the 0.7.1 milestone Mar 28, 2025
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.

2 participants