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

Implement manhattan and chebyshev distance for integer vectors #593

Merged
merged 8 commits into from
Dec 24, 2024

Conversation

tguichaoua
Copy link
Contributor

This implements #415 using the code generator.

Copy link
Owner

@bitshifter bitshifter left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

abs_diff on signed integers returns an unsigned integer so this will need a bit more work to code generate that .

The original #415 PR returned the result in a larger sized integer which for the Manhattan distance would help avoid overflow, however I guess for higher dimensions than 2 overflow would still be possible, so I'm not sure it's worth doing that. Also while glam doesn't currently have a I128Vec* or U128Vec* if they're ever added there is no u256 primitive in Rust which would make returning a larger integer in with any 128 sizes integer vector not possible. Give that I'm leaning towards your implementation and not returning a larger integer type, but adding a comment to the method pointing out that the result could overflow so that the caller is aware. Do you have any thoughts on how overflow should be handled for these?

Could you also add some tests for these new methods as part of this PR.

@tguichaoua
Copy link
Contributor Author

Do you have any thoughts on how overflow should be handled for these?

We should use unsigned integer as return type. As per the documentation of abs_diff:

This function always returns the correct answer without overflow or panics by returning an unsigned integer.

For manhattan_distance I added checked_manhattan_distance which returns None if an overflow would occurs.

@bitshifter bitshifter merged commit e8a53c6 into bitshifter:main Dec 24, 2024
18 checks passed
@bitshifter
Copy link
Owner

Thanks!

@tguichaoua tguichaoua deleted the vec2_int_distance branch December 24, 2024 06:26
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