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

Remove miniature scale from science data and fix segfault #126

Merged
merged 3 commits into from
Dec 23, 2021

Conversation

chapulina
Copy link
Contributor

Tests were crashing for that PR, while investigating, I noticed it happened because the MINIATURE_SCALE had been removed.

This PR removes the miniature scale and prints a warning instead of segfaulting, but I haven't looked closely into the barycentric interpolation to see why the miniature scale caused the issue, could you take a look @mabelzhang ?

I also added other changes from #88 which help us not need the miniature scale:

  • The invisible ground plane makes it possible to easily scroll to a far away distance
  • This can be seen with the 300kmx300km grid that is also added here
  • The increased camera clipping distances are also needed to see the entire grid from a distance
  • Added the camera controls plugin so the user can easily go back to the initial camera angle

miniature_scale

Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina self-assigned this Dec 22, 2021
Copy link
Collaborator

@mabelzhang mabelzhang left a comment

Choose a reason for hiding this comment

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

Thank you for debugging! Are you able to find data to test it locally? If I just delete the added if-statements, it still doesn't seg fault or hang for me.

I checked the hanging test at https://github.com/osrf/lrauv/pull/88/checks?sha=fa28e234918d0c2a86232d272e931095e2c1600c , and the output

10: [Wrn] [ScienceSensorsSystem.cc:858] 1D linear interpolation: query point lies on one side of all interpolation points. Cannot interpolate.

suggests that it returned NaN and something continued looping, possibly one of the loops in PostUpdate():

  for (auto &[entity, sensor] : this->entitySensorMap)

On a side note, the bad news of looking at this actual data is that, I now remember why we needed the per-slice searches as opposed to barycentric. Ughhhhhh. It's the z resolution being significantly smaller than x and y! The 4 NNs are all in 1 line along z. We might have to go back to trilinear interpolation. Ugh. At least I haven't deleted those branches.

To make our lives easier, we might have to take a step back and make the strict assumption that input indices are sorted, like in netCDF.

On a related note, debugging the interpolation will be significantly easier if we can publish a visual Marker from the ScienceSensorsSystem plugin. Anything against that?

The last thing I left off in the trilinear interpolation branch is that the found 4 NNs didn't satisfy the assumption that they need to be in a rectangular prism, which was the reason I moved to the less restrictive barycentric interpolation. I would publish a Marker to see if there are any bugs to shake out, before giving up and rewinding to assume all the input indices are sorted.

Verdict: real data is ugly.

Signed-off-by: Mabel Zhang <mabel@openrobotics.org>
Copy link
Collaborator

@mabelzhang mabelzhang left a comment

Choose a reason for hiding this comment

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

Should be fixed in b8641f0
So the cause was a pretty important bug - in the 1D case of interpolation, I never stored the array after sorting, only stored the sorted indices, heh.
I'm gonna approve my own fix 😅 Try it and see if the new warnings still print for you - they shouldn't anymore.

@chapulina
Copy link
Contributor Author

Warnings are gone! Thanks for fixing the bug :D

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.

2 participants