Skip to content

Fix intervaltree min max - #3

Merged
BenFrantzDale merged 4 commits into
Formlabs:masterfrom
BenFrantzDale:fix-intervaltree-min-max
Apr 3, 2019
Merged

BenFrantzDale merged 4 commits into
Formlabs:masterfrom
BenFrantzDale:fix-intervaltree-min-max

Conversation

@BenFrantzDale

Copy link
Copy Markdown

No description provided.

Comment thread IntervalTree.h
IntervalStartCmp());
if (!ivals.empty()) {
center = (minmaxStart.first->start + minmaxStop.second->stop) / 2;
{ // This scope is because afterward we sort and so these two iterators

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This is just scoping

Comment thread IntervalTree.h
@@ -160,15 +164,27 @@ class IntervalTree {

Scalar min() const {
assert(!empty());

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

These are the fixes.

Comment thread interval_tree_test.cpp
return Catch::Session().run( argc, argv );
}

TEST_CASE( "Test min/max" ) {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This tests the fixes.

@will-formlabs will-formlabs left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks good

@dbiswas2808 dbiswas2808 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM. Just a thought, is it a good idea to just a cache the min and max when we are building the tree for each level?

@BenFrantzDale
BenFrantzDale merged commit c1f7048 into Formlabs:master Apr 3, 2019
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