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

Add cross product operator #818

Merged
merged 6 commits into from
Jan 21, 2025
Merged

Add cross product operator #818

merged 6 commits into from
Jan 21, 2025

Conversation

mfzmullen
Copy link
Contributor

@mfzmullen mfzmullen commented Jan 8, 2025

Takes in two operators of size A0 x ... x An x 3 (or 2) and size B0 x ... x Bn x (3 or 2) with matching batch sizes and computes the cross product. If both input operators have size = 2 on the last dimension, the size of the output's last dimension is 1 and only contains the z component of the output. This is contrary, but as similar to, the NumPy implementation which drops the rank of the output by 1 if both inputs are in-plane vectors. Doing so in MatX does not seem straightforward since Rank is a static method and we may not know the sizes of A and B (e.g. 2 or 3 on the last dimension) at the time the rank is determined, but those sizes affect the rank of cross if we followed NumPy's approach.

Copy link
Collaborator

@cliffburdick cliffburdick left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for doing this

@cliffburdick
Copy link
Collaborator

/build

@cliffburdick
Copy link
Collaborator

Hi @mfzmullen, I apologize I didn't see this earlier, but the documentation needs to be updated:

/home/jenkins/workspace/unit-tests/include/matx/operators/cross.h:188: error: argument 'OpA' of command @param is not found in the argument list of matx::cross(const OpA &A, const OpB &B) (warning treated as error, aborting now)

@mfzmullen
Copy link
Contributor Author

@cliffburdick no worries, my bad for missing that in the first place! Just pushed

@cliffburdick
Copy link
Collaborator

/build

@cliffburdick
Copy link
Collaborator

Hi @mfzmullen now It's getting:

/home/jenkins/workspace/unit-tests/docs_input/api/linalg/other/cross.rst:16: WARNING: Include file '/home/jenkins/workspace/unit-tests/test/00_Operator/OperatorTests.cu' not found or reading it failed [docutils]`

You can test this yourself by "make sphinx" if you want it to run in a local environment.

@cliffburdick
Copy link
Collaborator

/build

@mfzmullen
Copy link
Contributor Author

@cliffburdick got the documentation built locally with latest push. Again my apologies on the mess with that, but thank you for your patience.

@cliffburdick
Copy link
Collaborator

/build

@cliffburdick cliffburdick merged commit 9b941ce into NVIDIA:main Jan 21, 2025
1 check passed
@cliffburdick
Copy link
Collaborator

Merged. Thanks @mfzmullen !

@cliffburdick
Copy link
Collaborator

@mfzmullen it looks like the unit test is failing:

[  FAILED  ] OperatorTestsFloatNonComplexAllExecs/4.Cross, where TypeParam = cuda::std::__4::tuple<matx::matxHalf<__nv_bfloat16>, matx::cudaExecutor>

@cliffburdick
Copy link
Collaborator

/build

@mfzmullen
Copy link
Contributor Author

@cliffburdick strange, I played with the tolerance a bit for the half types and wasn't getting any failures with the current setting after a few dozen runs. I can look at it a bit more.

@cliffburdick
Copy link
Collaborator

Here is the error:

Comparison failed at /home/jenkins/workspace/unit-tests/test/00_operators/OperatorTests.cu:2035:0/1: val=4.4375 file=4.4078 (out)
/home/jenkins/workspace/unit-tests/test/00_operators/OperatorTests.cu:2035: Failure
Failed
[  FAILED  ] OperatorTestsFloatNonComplexAllExecs/4.Cross, where TypeParam = cuda::std::__4::tuple<matx::matxHalf<__nv_bfloat16>, matx::cudaExecutor> (0 ms)

@mfzmullen
Copy link
Contributor Author

@cliffburdick if I change the threshold to 0.06 for the half types and run
./test/matx_test --gtest_filter="*Cross*" --gtest_repeat=1000 --gtest_break_on_failure

I had it pass twice and failed once after 623 iterations. What failure probability is acceptable?

@cliffburdick
Copy link
Collaborator

I don't think there would be any uncertainty in these calculations, so I wouldn't expect repeating to help. 0.06 should be fine.

@mfzmullen
Copy link
Contributor Author

While there isn't an uncertainty, each repetition uses different randomly generated inputs, so the difference between the numpy and matx versions will change on each repetition. I'll push a change soon.

@cliffburdick
Copy link
Collaborator

NumPy can use the same seed for the rng so you get the same number each time. That's how we do most of our tests currently so we have some predictability.

@mfzmullen
Copy link
Contributor Author

Oh got it. I see that in some of the generators now, but not in the ones I used as examples (toeplitz). I can add that to my PR.

@cliffburdick
Copy link
Collaborator

It looks like it's still failing with a larger threshold. I will make it even larger since it appears to be just a precision issue:

Comparison failed at /home/jenkins/workspace/unit-tests/test/00_operators/OperatorTests.cu:2035:2/2: val=-4 file=-4.02057 (out)
/home/jenkins/workspace/unit-tests/test/00_operators/OperatorTests.cu:2035: Failure

@mfzmullen
Copy link
Contributor Author

That difference is already less than thresh (=0.08). That is with a double or float perhaps? All these tests passed on my local machine.

The tests need to pass before merging to main or no? Just curious why they seem to be failing sometimes even with the consistent seed in the rng now.

@cliffburdick
Copy link
Collaborator

It looks like it's working now. Thanks!

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