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

Unit-Safe Implementation of std::hypot #384

Merged

Conversation

justin-kottinger
Copy link
Contributor

Summary

Implements a unit-safe version of std::hypot within the AU library. This implementation ensures dimensional consistency when calculating the hypotenuse/Euclidean norm while preserving the underlying units.

Implementation Details

  • Extends the existing AU library with a unit-safe wrapper around std::hypot
  • Maintains dimensional analysis correctness throughout calculations
  • Returns results in the common unit and underlying representation as the input parameters

Related to #383

@CLAassistant
Copy link

CLAassistant commented Feb 21, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@chiphogg chiphogg 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 making this contribution! I'll re-review once you get the CLA signed, and remove the QuantityPoint overloads.

Copy link
Contributor

@chiphogg chiphogg left a comment

Choose a reason for hiding this comment

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

This LGTM with just one more small change needed. (I spotted the change manually, and then in running the jobs, it looks like it is in fact causing failures.)

You'll also need to sign the CLA. Let me know if you're having any difficulties with that, and I'll try to help troubleshoot!

@justin-kottinger
Copy link
Contributor Author

Thanks for the speedy review. Regarding the CLA, I signed the corporate version on Friday. It should be under my name with the company name of "Swarmbotics AI". It does not look like CLAassistant is finding it. Do I need to sign the individual one as well?

@chiphogg
Copy link
Contributor

Thanks for the speedy review. Regarding the CLA, I signed the corporate version on Friday. It should be under my name with the company name of "Swarmbotics AI". It does not look like CLAassistant is finding it. Do I need to sign the individual one as well?

I have reached out internally to the folks who manage our CLA assistant. I hope we can get this cleared up pretty soon.

@timhirsh
Copy link
Member

timhirsh commented Feb 24, 2025

Hi @justin-kottinger, thanks for your contribution here! For corporate CLA's, our options are to manually allowlist either:

  • a public GitHub organization. this would allow all public members of the organization to contribution to Aurora Opensource on behalf of the corporate CLA that you signed earlier.
  • individual usernames of members of the organization "Swarmbotics AI"

Does "Swarmbotics AI" have a public GitHub organization that we can list here? That would be preferable over maintaining a list of usernames. Thanks!

[EDIT] I've found your fork here: https://github.com/SwarmboticsAI/au (which I missed earlier), so I'll use that 👍 The next step is to make yourself a public member of that org. Right now I'm seeing No public members, so the CLA Assistant is not able to verify your membership of this org. Can you please update your org visibility please?

@chiphogg
Copy link
Contributor

Also, note that while looking into this internally, we found that you're the first contributor to use the corporate CLA. 😄 So thanks for helping us work out the kinks, and I hope it explains why there are any kinks!

@justin-kottinger
Copy link
Contributor Author

justin-kottinger commented Feb 25, 2025

Thank you @timhirsh and @chiphogg for your help on this. My team generally prefers to keep our memberships private for security reasons. However, I have temporarily made my membership public to help with this PR. All tests are passing now. 🎉 Please let me know if you need anything else.

@chiphogg chiphogg merged commit 11fe374 into aurora-opensource:main Feb 25, 2025
15 checks passed
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.

4 participants