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

Return numerically maximal index (unsigned) or -1 (signed) for no match case #79

Merged
merged 7 commits into from
Nov 3, 2017

Conversation

magehrig
Copy link
Contributor

@magehrig magehrig commented Nov 3, 2017

Address issue #2.



//! invalid index
static constexpr auto InvalidIndex = std::numeric_limits<IT>::max();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Once you've converted this to a templated constexpr inline function, you could return as value :

std::is_unsigned<IT>::value ? std::numeric_limits<IT>::max() : IT(-1); 

Copy link
Collaborator

@HannesSommer HannesSommer left a comment

Choose a reason for hiding this comment

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

Lgtm, except for the failing unit tests.
Did you find already how to fix those?

@@ -213,7 +213,17 @@ namespace Nabo
#define NABO_VERSION "1.0.6"
//! version of the Nabo library as an int
#define NABO_VERSION_INT 10006


Copy link
Collaborator

Choose a reason for hiding this comment

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

If you make another commit, could you add a comment here, like:

// TODO (c++14) Convert invalidIndex, invalidValue to constexpr templated variables

@HannesSommer
Copy link
Collaborator

HannesSommer commented Nov 3, 2017

Aha, I believe you've found a bug in the unit tests:
In
https://github.com/ethz-asl/libnabo/blob/b1ac136d08554bb74b96395f6cc5226b13e5cae9/tests/knnvalidate.cpp#L181
there should have been T instead of float.

But now there should probably be invalidValue<T>() or the ::InvalidValue if accessible.

Copy link
Collaborator

@HannesSommer HannesSommer left a comment

Choose a reason for hiding this comment

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

Great, that should do.
Thanks!
@simonlynen , @stephanemagnenat , any concerns? E.g. style-wise?

const float distDiff(fabsf((pbf-pq).squaredNorm() - (pkdtree-pq).squaredNorm()));
if (distDiff > numeric_limits<float>::epsilon())
const T distDiff(fabsf((pbf-pq).squaredNorm() - (pkdtree-pq).squaredNorm()));
if (distDiff > numeric_limits<T>::epsilon())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch :)

@magehrig
Copy link
Contributor Author

magehrig commented Nov 3, 2017

I first have to find out why the tests do not pass

Copy link
Collaborator

@HannesSommer HannesSommer left a comment

Choose a reason for hiding this comment

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

Yes, I was a bit optimistic here. Strange. I can also take over from here, if you've run out of time..

@magehrig
Copy link
Contributor Author

magehrig commented Nov 3, 2017

Would be nice if you take a quick look at the test. What i have found out so far is that indexes_kdtree is filled with -1 and dists2_kdtree is filled with 0 if j == 1u; j > 1u succeeds.

@HannesSommer
Copy link
Collaborator

Okay, I'll do that. Not now but later, probably weekend. Thanks!

@HannesSommer HannesSommer self-assigned this Nov 3, 2017
@magehrig
Copy link
Contributor Author

magehrig commented Nov 3, 2017

@HannesSommer Should be fixed now ;)

Copy link
Collaborator

@HannesSommer HannesSommer left a comment

Choose a reason for hiding this comment

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

Great! (And I though, I had checked all of these - apparently not really...)

@HannesSommer HannesSommer changed the title Return numerically maximal index for no match case Return numerically maximal index (unsigned) or -1 (signed) for no match case Nov 3, 2017
@magehrig magehrig merged commit 34e2cc0 into master Nov 3, 2017
@magehrig magehrig deleted the feature/max-index-no-match branch November 3, 2017 19:53
@HannesSommer HannesSommer removed their assignment Nov 4, 2017
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