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 linear interpolation methods. #412

Merged
merged 28 commits into from
May 2, 2022
Merged

Add linear interpolation methods. #412

merged 28 commits into from
May 2, 2022

Conversation

arjo129
Copy link
Contributor

@arjo129 arjo129 commented Apr 18, 2022

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

🎉 New feature

This PR adds linear interpolation methods to the ignition math library these interpolation methods are needed for the VolumetricScalarGrid field,.

Signed-off-by: Arjo Chakravarty [email protected]

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

The following PR adds a datastructure that represents a sparse volumetric grid. It allows for storing data in 3D.

# TODO
- [ ] Write unit tests
- [ ] docs

Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
This PR adds linear interpolation methods to the ignition math library these interpolation methods are needed for the VolumetricScalarGrid field,.

Signed-off-by: Arjo Chakravarty <[email protected]>
@chapulina chapulina added the MBARI-LRAUV Sponsored by MBARI-LRAUV project: https://github.com/osrf/lrauv label Apr 18, 2022
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
@codecov
Copy link

codecov bot commented Apr 19, 2022

Codecov Report

Merging #412 (4be2adb) into main (a8a5b94) will decrease coverage by 0.06%.
The diff coverage is n/a.

❗ Current head 4be2adb differs from pull request most recent head 116a045. Consider uploading reports for the commit 116a045 to get more accurate results

@@            Coverage Diff             @@
##             main     #412      +/-   ##
==========================================
- Coverage   99.90%   99.84%   -0.07%     
==========================================
  Files          46       49       +3     
  Lines        4260     4396     +136     
==========================================
+ Hits         4256     4389     +133     
- Misses          4        7       +3     
Impacted Files Coverage Δ
...e/ignition/math7/ignition/math/detail/AxisIndex.hh 100.00% <0.00%> (ø)
...n/math7/ignition/math/VolumetricGridLookupField.hh 94.82% <0.00%> (ø)
...n/math7/ignition/math/detail/InterpolationPoint.hh 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8a5b94...116a045. Read the comment docs.

Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
@arjo129 arjo129 changed the base branch from arjo/feat/volumetric_grid_field to main April 21, 2022 15:10
@arjo129 arjo129 marked this pull request as ready for review April 21, 2022 15:10
@arjo129 arjo129 requested a review from scpeters as a code owner April 21, 2022 15:10
@chapulina chapulina added the 🌱 garden Ignition Garden label Apr 22, 2022
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

Another pass. Bottom line, code looks good but a few more tests would nice.

include/ignition/math/detail/InterpolationPoint.hh Outdated Show resolved Hide resolved
include/ignition/math/detail/InterpolationPoint.hh Outdated Show resolved Hide resolved
include/ignition/math/detail/InterpolationPoint.hh Outdated Show resolved Hide resolved
src/InterpolationPoint_TEST.cc Outdated Show resolved Hide resolved
src/InterpolationPoint_TEST.cc Outdated Show resolved Hide resolved
src/VolumetricGridLookupField_TEST.cc Show resolved Hide resolved
src/VolumetricGridLookupField_TEST.cc Show resolved Hide resolved
src/VolumetricGridLookupField_TEST.cc Outdated Show resolved Hide resolved
src/VolumetricGridLookupField_TEST.cc Show resolved Hide resolved
src/VolumetricGridLookupField_TEST.cc Outdated Show resolved Hide resolved
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

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

LGTM barring a few comments. Also, if we can further reduce all tolerances used in tests, that'd be great :)

include/ignition/math/VolumetricGridLookupField.hh Outdated Show resolved Hide resolved
/// interpolation.
public: std::vector<InterpolationPoint1D<T>> GetInterpolators(
const T &_value,
double _tol = 1e-6) const
Copy link
Contributor

Choose a reason for hiding this comment

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

@arjo129 should _tol be of type T?

// Out of range
return {};
}
else if (fabs(it->first - _value) < _tol)
Copy link
Contributor

Choose a reason for hiding this comment

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

@arjo129 nit^N!: if we use abs instead of fabs, including an using std::abs; statement before the if-else clause, T can be any numeric value, not just float and double.

include/ignition/math/detail/InterpolationPoint.hh Outdated Show resolved Hide resolved
include/ignition/math/detail/InterpolationPoint.hh Outdated Show resolved Hide resolved
@hidmic
Copy link
Contributor

hidmic commented Apr 26, 2022

CI failures seem unrelated. We are lacking some test coverage on VolumetricGridLookupField.hh though.

Signed-off-by: Arjo Chakravarty <[email protected]>
Signed-off-by: Arjo Chakravarty <[email protected]>
@arjo129 arjo129 enabled auto-merge (squash) April 28, 2022 03:08
@arjo129 arjo129 merged commit 9622e9e into main May 2, 2022
@arjo129 arjo129 deleted the arjo/feat/interpolation branch May 2, 2022 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌱 garden Ignition Garden MBARI-LRAUV Sponsored by MBARI-LRAUV project: https://github.com/osrf/lrauv
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants