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 support for Eigen::Transform #1555

Merged
merged 8 commits into from
Jan 7, 2025
Merged

Conversation

WanZhiQiu-ac
Copy link
Contributor

No description provided.

@WanZhiQiu-ac
Copy link
Contributor Author

The unit test failed at expect(buffer == "[-0.3492063492063493,0.9206349206349207,0.17460317460317448,0,0.0317460317460318,-0.17460317460317443,0.9841269841269841,0,0.9365079365079365,0.3492063492063492,0.03174603174603163,0,1,2,3,1]"); on gcc-x86. I suspect it might be due to a precision issue. Feel free to make any necessary edits.

@WanZhiQiu-ac
Copy link
Contributor Author

Now I am certain it's a precision issue. I checked other unit tests, and none of them have such high-precision testing, so I removed the high-precision unit test. Perhaps this issue should be addressed in a separate PR? Or maybe it doesn't need to be fixed?

@stephenberry
Copy link
Owner

I don't think this is a Glaze issue, because it is robustly tested for floating point round-tripping (every possible float is tested and random doubles). This is probably a precision issue with the way floating point assembly is generated for Eigen. Floating point math operations with Eigen aren't guaranteed to give the exact same result across compilers.

@stephenberry
Copy link
Owner

Thanks for adding this support! Do you think it is ready to be merged?

@WanZhiQiu-ac
Copy link
Contributor Author

I don't think this is a Glaze issue, because it is robustly tested for floating point round-tripping (every possible float is tested and random doubles). This is probably a precision issue with the way floating point assembly is generated for Eigen. Floating point math operations with Eigen aren't guaranteed to give the exact same result across compilers.

Totally agree

@WanZhiQiu-ac
Copy link
Contributor Author

And yes I think it is ready to be merged

@stephenberry stephenberry merged commit 0bc0c5f into stephenberry:main Jan 7, 2025
12 checks passed
@stephenberry
Copy link
Owner

Sweet! Merged it in.

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