BUG: Fix regression in PolygonSpatialObject::IsInsideInObjectSpace by rflanz · Pull Request #2422 · InsightSoftwareConsortium/ITK

rflanz · 2021-03-18T20:42:58Z

This is a followup to the bugfix for issue #1082 where there was a
regression in the code on determining the inside/outsideness of points.

There are some cases where the algorithm still fails as there are two
boundary checks in the code which should be identical.

Test on adjacent segment pairs from start to finish
Test on segment pair from finish point to start point on closed
polygons.

The previous bugfix fixed the code in item 1, but did not update the
check (which should be identical in item 2).

This should address this issue making both boundary on segments the same.

PR Checklist

No API changes were made (or the changes have been approved)
No major design changes were made (or the changes have been approved)
Added test (or behavior not changed)
Updated API documentation (or API not changed)
Added license to new files (if any)
Added Python wrapping to new files (if any) as described in ITK Software Guide Section 9.5
Added ITK examples for all new major features (if any)

Refer to the ITK Software Guide for
further development details if necessary.

rflanz · 2021-03-18T20:45:25Z

I have not created a test for this yet, but it is possible maybe to create one from the same tests that we found this issue in. Please let me know if adding a test is required.

dzenanz

A fix is better than no fix, but a test is highly desirable.

thewtex · 2021-03-19T19:23:26Z

👍 for a test to ensure the issue has been addressed and stays addressed.

thewtex · 2021-03-19T19:23:42Z

@rflanz thanks for the contribution!

dzenanz

Great effort! This only needs minor changes given in-line.

Leengit · 2021-03-24T14:14:51Z

You might replace auto with the expected types in some places in the testing code. That way the testing code will also be testing that these types are not changing. Of course, that's not always worth the trouble, especially in cases where the expected type is something hard to write out -- your call.

jhlegarreta

Thanks for doing this @rflanz !! Testing is essential to any software and ITK strives to extensively test its code base.

A few inline comments with the aim of improving the style and making it consistent with the ITK coding style: https://itk.org/ItkSoftwareGuide.pdf (Appendix Three; more especially Section C.23).

Let me know if I can help somehow.

Keep up the good work 💯.

jhlegarreta

@rflanz thanks for having addressed many of the previous review comments. A few more. Hope they make sense. Thanks.

jhlegarreta

Starting to converge ⏩. Another inline comment.

Thanks for the patience and hard work @rflanz !

This is a followup to the bugfix for issue InsightSoftwareConsortium#1082 where there was a regression in the code on determining the inside/outsideness of points. There are some cases where the algorithm still fails as there are two boundary checks in the code which should be identical. 1. Test on adjacent segment pairs from start to finish 2. Test on segment pair from finish point to start point on closed polygons. The previous bugfix fixed the code in item 1, but did not update the check (which should be identical in item 2). This should address this issue making both boundary on segments the same. BUG: Add testing for regression itkPolygon::ObjectIsInsideObjectSpaceTest Regression STYLE: ApplyClangFormat STYLE: fix white space in CMakeLists.txt STYLE: Adjust text STYLE: Address Pull Request Comments STYLE: Follow somore more style guidelines STYLE: address more PR comments

jhlegarreta · 2021-03-26T21:50:42Z

/azp run ITK.Windows.Python

jhlegarreta · 2021-03-27T00:40:49Z

Not sure why the ITK.Windows.Python build is failing. This PR does not change the class definition or the wrap file:
https://open.cdash.org/viewBuildError.php?buildid=7125016

Circle CI looks to be having other types of issues, as the message There was an infrastructure problem while running your tests reports.

I've noticed that this branch is 446 commits behind master so rebasing on master will already be a good thing IMHO.

Cannot investigate more than that.

dzenanz · 2021-03-27T00:46:32Z

This is a PR based on 5.1.2 into release branch.

rflanz · 2021-03-29T13:45:22Z

Okay, I am not clear on what needs to be done next. Have I asked for merge to the wrong branch? Let me know if there is anything I can do, and I will get on top of it. Thanks.

dzenanz · 2021-03-29T13:50:45Z

I think the failures are unrelated to the PR. I checked it out and I will build it locally.

dzenanz · 2021-03-29T13:51:12Z

I will also probably make a variant for the master branch at the same time.

dzenanz · 2021-03-29T13:54:16Z

Multi-line comment issue has been fixed in master. And Python build problem cannot open file 'python39.lib' has nothing to do with this PR.

dzenanz · 2021-03-29T15:11:45Z

I didn't have permission to push to your fork @rflanz, so I made a new PR (#2453). It uses a compressed baseline image (1KB vs 34KB). It also puts the test data in the local directory (which is the new style).

rflanz · 2021-03-29T15:18:48Z

Ok, Great Thanks to all of the team on ITK.
I was a bit daunted at first by the idea of submitting, but it has proved to be a pleasant experience.
To all who helped out: Thanks for taking the time to help and review.
It is most appreciated.

jhlegarreta · 2021-03-29T15:37:49Z

Ok, Great Thanks to all of the team on ITK.
I was a bit daunted at first by the idea of submitting, but it has proved to be a pleasant experience.
To all who helped out: Thanks for taking the time to help and review.
It is most appreciated.

@rflanz glad to read this 👍 . Fixes and contributions are always welcome. I grant that our software process is at times demanding. We're happy to help. And sorry not having given approval; I was cautious about the failing checks.

dzenanz · 2021-03-30T15:55:36Z

Closing in favor of #2453.

rflanz changed the base branch from master to release March 18, 2021 20:47

dzenanz reviewed Mar 18, 2021

View reviewed changes

rflanz marked this pull request as draft March 22, 2021 17:48

dzenanz reviewed Mar 24, 2021

View reviewed changes

rflanz force-pushed the PolygonSpatalObjectRegression-fix branch from 1750442 to 6740044 Compare March 25, 2021 19:22

jhlegarreta reviewed Mar 26, 2021

View reviewed changes

rflanz force-pushed the PolygonSpatalObjectRegression-fix branch from 6740044 to 5f6e558 Compare March 26, 2021 15:01

jhlegarreta self-requested a review March 26, 2021 15:13

jhlegarreta reviewed Mar 26, 2021

View reviewed changes

Comment thread Modules/Core/SpatialObjects/test/itkPolygonSpatialObjectIsInsideInObjectSpaceTest.cxx Outdated

Comment thread Modules/Core/SpatialObjects/test/itkPolygonSpatialObjectIsInsideInObjectSpaceTest.cxx Outdated

rflanz force-pushed the PolygonSpatalObjectRegression-fix branch from 5f6e558 to b332cd1 Compare March 26, 2021 16:04

jhlegarreta reviewed Mar 26, 2021

View reviewed changes

Comment thread Modules/Core/SpatialObjects/test/itkPolygonSpatialObjectIsInsideInObjectSpaceTest.cxx Outdated

rflanz force-pushed the PolygonSpatalObjectRegression-fix branch from b332cd1 to 2514f83 Compare March 26, 2021 20:18

jhlegarreta self-requested a review March 26, 2021 20:47

dzenanz mentioned this pull request Mar 29, 2021

BUG: Fix regression in PolygonSpatialObject::IsInsideInObjectSpace #2453

Merged

dzenanz closed this Mar 30, 2021

Uh oh!

Conversation

rflanz commented Mar 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Uh oh!

rflanz commented Mar 18, 2021

Uh oh!

dzenanz left a comment

Choose a reason for hiding this comment

Uh oh!

thewtex commented Mar 19, 2021

Uh oh!

thewtex commented Mar 19, 2021

Uh oh!

dzenanz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Leengit commented Mar 24, 2021

Uh oh!

jhlegarreta left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jhlegarreta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jhlegarreta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jhlegarreta commented Mar 26, 2021

Uh oh!

jhlegarreta commented Mar 27, 2021

Uh oh!

dzenanz commented Mar 27, 2021

Uh oh!

rflanz commented Mar 29, 2021

Uh oh!

dzenanz commented Mar 29, 2021

Uh oh!

dzenanz commented Mar 29, 2021

Uh oh!

dzenanz commented Mar 29, 2021

Uh oh!

dzenanz commented Mar 29, 2021

Uh oh!

rflanz commented Mar 29, 2021

Uh oh!

jhlegarreta commented Mar 29, 2021

Uh oh!

dzenanz commented Mar 30, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rflanz commented Mar 18, 2021 •

edited

Loading

jhlegarreta left a comment •

edited

Loading