-
Notifications
You must be signed in to change notification settings - Fork 677
[wpiunit] Add hypot method for calculating the hypotenuse between two distances (second trial) #8501
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
base: main
Are you sure you want to change the base?
Conversation
|
I'm confused as to why this PR was recreated, when it looks like it's the same commits. |
|
It is not the same. The previous implementation of hypot() was using Measure with some other things, which was wrong. Like the implementation itself was wrong, I think. And I added some more detail to the comments. I think it should work now, and yet it still didn't pass the tests. Here is a more clear comparison of the static old version: I might be wrong. |
|
Now I see the problem. I pushed my new commit on the same repo, so the PR #8495 was also updated with the new commit. I thought I had to create a new PR with the new commit. Because when I did the PR #8495 I originally only had the commit a2dcf09. So you are right, this was an unnecessary extra PR that I hadn't realized. Sorry about that, I didn't know PRs worked this way. |
| */ | ||
| static Distance hypot(Distance a, Distance b) { | ||
| return a.unit().ofBaseUnits( | ||
| Math.hypot(a.in(a.unit().getBaseUnit()), b.in(b.unit().getBaseUnit())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it not use the same unit for both values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean? I am confused. It uses Distance for both of the parameters and the return value.
Adds a simple .hplot() method with tests that gets the hypothenuse between two distances. Contains both static and instance versions. Fixes #7666