Skip to content

feat: improve range reduction algorithm#169

Open
vishwa2710 wants to merge 5 commits intomainfrom
users/devin/improved-range-reduction-algorithm
Open

feat: improve range reduction algorithm#169
vishwa2710 wants to merge 5 commits intomainfrom
users/devin/improved-range-reduction-algorithm

Conversation

@vishwa2710
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Sep 12, 2023

Codecov Report

Merging #169 (2ebde64) into main (82d4af1) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #169      +/-   ##
==========================================
+ Coverage   79.86%   79.88%   +0.02%     
==========================================
  Files          93       93              
  Lines        7448     7447       -1     
==========================================
+ Hits         5948     5949       +1     
+ Misses       1500     1498       -2     
Files Coverage Δ
...c/OpenSpaceToolkit/Physics/Units/Derived/Angle.cpp 72.99% <100.00%> (-0.08%) ⬇️

... and 1 file with indirect coverage changes

@@ -1,5 +1,5 @@
/// Apache License 2.0

#include <cmath>
Copy link
Contributor

Choose a reason for hiding this comment

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

Simply for consistency:

Suggested change
#include <cmath>
#include <cmath>

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const Real adjestedRangeValue = aRangeLowerBound + excessValue;
const Real adjustedRangeValue = aRangeLowerBound + excessValue;

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if ((adjestedRangeValue >= aRangeLowerBound) && (adjestedRangeValue < aRangeUpperBound))
if ((adjustedRangeValue >= aRangeLowerBound) && (adjustedRangeValue < aRangeUpperBound))

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return adjestedRangeValue;
return adjustedRangeValue;

Copy link
Contributor

Choose a reason for hiding this comment

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

No need for an else block, since the if block returns.

Copy link
Contributor

Choose a reason for hiding this comment

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

Comment block style is likely different from the rest of the codebase?

Copy link
Contributor Author

@vishwa2710 vishwa2710 left a comment

Choose a reason for hiding this comment

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

Just some nits. yeah I agree this is a bit gross :/ let me think if there's a better way to deal with the rounding issues.

return false;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change

}

return this->accessValue() * Angle::SIRatio(unit_) / Angle::SIRatio(aUnit);
return (this->accessValue() * Angle::SIRatio(unit_) / Angle::SIRatio(aUnit));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
return (this->accessValue() * Angle::SIRatio(unit_) / Angle::SIRatio(aUnit));
return this->accessValue() * Angle::SIRatio(unit_) / Angle::SIRatio(aUnit);

);
}
// this line handles some rounding error albeit in a way that doesnt pass the smell test...
Real value = aValue + 10.0 - 10.0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
Real value = aValue + 10.0 - 10.0;
const Real value = aValue + 10.0 - 10.0;

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.

3 participants

Comments