Skip to content

Adds a Time Interpolation API#456

Merged
arjo129 merged 21 commits intomainfrom
arjo/feat/time_interpolation
Aug 5, 2022
Merged

Adds a Time Interpolation API#456
arjo129 merged 21 commits intomainfrom
arjo/feat/time_interpolation

Conversation

@arjo129
Copy link
Copy Markdown
Contributor

@arjo129 arjo129 commented Jul 8, 2022

🎉 New feature

Closes #

Summary

This PR adds a TimeVaryingVolumetricGrid. The core idea is this extends VolumetricGrid by stepping through time. It uses template speciallization to leave the room open to custom implementations of the index.

Test it

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • 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.

This Commit adds a `TimeVaryingVolumetricGrid`. The core idea is this extends `VolumetricGrid` by stepping through time. It uses template speciallization to leave the room open to custom implementations of the index.

Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@github-actions github-actions bot added the 🌱 garden Ignition Garden label Jul 8, 2022
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129 arjo129 added the MBARI-LRAUV Sponsored by MBARI-LRAUV project: https://github.com/osrf/lrauv label Jul 8, 2022
Copy link
Copy Markdown
Contributor

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

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

If you don't have the time to generate the Python interface. Do you mind to open a issue ?

@codecov
Copy link
Copy Markdown

codecov bot commented Jul 8, 2022

Codecov Report

Merging #456 (75125e1) into main (b9ae648) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 75125e1 differs from pull request most recent head 2db460d. Consider uploading reports for the commit 2db460d to get more accurate results

@@           Coverage Diff           @@
##             main     #456   +/-   ##
=======================================
  Coverage   99.69%   99.70%           
=======================================
  Files          75       77    +2     
  Lines        6912     7007   +95     
=======================================
+ Hits         6891     6986   +95     
  Misses         21       21           
Impacted Files Coverage Δ
include/gz/math/detail/InterpolationPoint.hh 100.00% <ø> (ø)
include/gz/math/TimeVaryingVolumetricGrid.hh 100.00% <100.00%> (ø)
...de/gz/math/TimeVaryingVolumetricGridLookupField.hh 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Copy Markdown
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.

@arjo129 First, high-level pass. As I try to visualize how everything comes together, a few pain points start creeping in.

arjo129 added 2 commits July 17, 2022 17:36
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
This commit adds a container class for values also. In particular it implements the behaviour for the class `TimeVaryingVolumetricGrid<T, V, InMemorySession<T, double>>` . This class serves as a lookup table for time varying items. To construct a `TimeVaryingVolumetricGrid` it is advised to use the `InMemoryTimeVaryingVolumetricGridFactory` to construct the actual grid from a given dataset. Do not that this dataset should fit in memory.

To progress along the time axis use the `InMemorySession` object. This can be acquired using the `CreateSession` function. To set the time to be queried, step the `InMemorySession`
using the `StepTo` function. Finally to look up a value use the `LookUp` function.

## Example

```
 InMemoryTimeVaryingVolumetricGridFactory<double, double> gridFactory;

  for (double t = 0; t < 1; t+=0.2)
  {
    for (double x = 0; x < 1; x+=0.5)
    {
      for (double y = 0; y < 1; y+=0.5)
      {
        for (double z = 0; z < 1; z+=0.5)
        {
          gridFactory.AddPoint(t, Vector3d{x,y,z}, t);
        }
      }
    }
  }

 /// Construct the grid
 auto grid = gridFactory.Build();

 /// Get the session cursor
 auto session = grid.CreateSession();

 /// Step to the correct time
 auto new_sess = grid.StepTo(session, 0.5);

 /// Retrieve value from given location
 auto  val = grid.LookUp(new_sess.value(), Vector3d{0.5, 0.5, 0.5});
```

## Other Changes in this commit
This commit also makes some changes to `VolumetricGridLookUpField`. In particular the changes allow
 `VolumetricGridLookUpField` to use custom indices and support looking up in larger
index slices. There is also a new constructor for it to support the `TimeVaryingVolumetricGridLookupField`. This shouldn;t really be a problem as the `VolumetricGridField` is only available on the main branch and not in any collection yet.

## TODO
There is still a fair deal of work to do by ways of documentation and test coverage. I would also like to change the behaviour of what happens when we go out of bounds of the grid.

Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@chapulina chapulina added Breaking change Breaks API, ABI or behavior. Must target unstable version. enhancement New feature or request labels Jul 25, 2022
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
arjo129 added a commit that referenced this pull request Jul 27, 2022
This PR generifies the index term. It also adds a constructor which is used by #456.

Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129 arjo129 changed the base branch from main to arjo/feat/volumetric_grid_custom_constructor July 27, 2022 07:13
@hidmic hidmic mentioned this pull request Jul 27, 2022
22 tasks
arjo129 added 2 commits July 28, 2022 10:17
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
arjo129 added a commit that referenced this pull request Jul 28, 2022
This PR generifies the index term. It also adds a constructor which is used by #456.
Base automatically changed from arjo/feat/volumetric_grid_custom_constructor to main July 28, 2022 03:01
arjo129 added 7 commits July 28, 2022 11:48
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129 arjo129 changed the title Adds a Time Interpolation API [WIP] Adds a Time Interpolation API Jul 28, 2022
@arjo129 arjo129 removed the Breaking change Breaks API, ABI or behavior. Must target unstable version. label Jul 29, 2022
arjo129 added 2 commits July 29, 2022 10:14
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129 arjo129 requested a review from hidmic July 29, 2022 05:10
namespace math
{
/// \brief Lookup table for a time-varying volumetric dataset.
/// This is an unimplimented template as the actual methods depend on the
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@arjo129 typo:

Suggested change
/// This is an unimplimented template as the actual methods depend on the
/// This is an unimplemented template as the actual methods depend on the

{
InterpolationPoint1D<T>
pt1{_session.iter->first, 0}, pt2{next->first, 1};
// If either has value interpolate using default value.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@arjo129 meta: ok with this behavior for a first pass, but it may not be appropriate for several use cases. Perhaps we can add a ticket to generalize this a bit (e.g. to allow constant extrapolation or simply return nullopt or whatever you can think of).

arjo129 added 2 commits August 3, 2022 10:28
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
}

/// Got nothing to interpolate. Out of bounds.
if (_interpolators[0].timeSlice.size() == 0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@arjo129 FYI, safe to ignore: there's std::vector<T, Allocator>::empty too.

{
// Get data at T=0
auto session = timeVaryingField.CreateSession();
auto points = timeVaryingField.LookUp(session, Vector3d{0.5, 0.5, 0.5});
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@arjo129 nit^2: having a const tolerance = Vector3d{0.5, 0.5, 0.5} right above may improve readability a bit.

ASSERT_EQ(points.size(), 2);
ASSERT_EQ(points[0].time, 0);
ASSERT_EQ(points[1].time, 1);
auto res = timeVaryingField.EstimateQuadrilinear<double>(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@arjo129 nit: I'd think it's not necessary to specify the template parameter here? Compiler should be able to infer X from arguments.

@hidmic
Copy link
Copy Markdown
Contributor

hidmic commented Aug 3, 2022

@arjo129 we got some linter errors in CI

Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
@arjo129
Copy link
Copy Markdown
Contributor Author

arjo129 commented Aug 4, 2022

Seems like homebrew is not passing for unknown reasons.

@hidmic
Copy link
Copy Markdown
Contributor

hidmic commented Aug 4, 2022

Seems like homebrew is not passing for unknown reasons.

@scpeters any ideas? We'd like to get this into main soon.

@hidmic
Copy link
Copy Markdown
Contributor

hidmic commented Aug 4, 2022

@arjo129 looks like it was a hiccup. We're good to go.

@arjo129 arjo129 merged commit 0a438f7 into main Aug 5, 2022
@arjo129 arjo129 deleted the arjo/feat/time_interpolation branch August 5, 2022 07:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request 🌱 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.

6 participants