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

ENH: Improve itkEuclideanDistancePointMetric coverage. #113

Conversation

hjmjohnson
Copy link
Member

Exercise basic object methods.

Exercise all Set/Get macros.

Refactor the test to accept input parameters to test all possible
conditions.

@hjmjohnson hjmjohnson self-assigned this Nov 6, 2018
@hjmjohnson
Copy link
Member Author

@jhlegarreta This has been stale for 2 years now waiting to make it perfect. It seems like the current additions are worthy of inclusion, and the additional work can be a separate PR.

@hjmjohnson
Copy link
Member Author

@hjmjohnson
Copy link
Member Author

Jon Hait... Gorroño
Uploaded patch set 1.Nov 10, 2016

Kitware Build Robot
Patch Set 1: Verified+1 Build Successful: CDash filtered results: https://open.cdash.org/index.php?&project=Insight&filtercount=3&field1=buildname/string&compare1=63&value1=21750-1&field2=buildstarttime/date&compare2=83&value2=2015-03-01&field3=buildstarttime/date&compare3=84&value3=2029-01-01Nov 10, 2016

Davis Vigneault
Patch Set 1: Code-Review+1 LGTM. +1Nov 11, 2016

Jon Hait... Gorroño
Patch Set 1: Hi Davis, thanks for the review ! Before it gets merged, though, the test requires some more work to exercise the Set/Get methods for the DistanceMap. I hope to work on it in the next few days.Nov 12, 2016

Hans J. Johnson

@hjmjohnson hjmjohnson closed this Nov 6, 2018
@hjmjohnson hjmjohnson deleted the ImproveEuclideanDistancePointMetricCoverage branch November 6, 2018 14:49
@hjmjohnson hjmjohnson restored the ImproveEuclideanDistancePointMetricCoverage branch November 6, 2018 19:24
@hjmjohnson
Copy link
Member Author

Accidently deleted/closed branch when the script for moving from Gerrit went haywire.

@hjmjohnson hjmjohnson reopened this Nov 6, 2018
Copy link
Member

@jhlegarreta jhlegarreta left a comment

Choose a reason for hiding this comment

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

This is not ready to be merged: it should be rebased on master (e.g. typedefs around), and as said in the follow-up it required more work. So let's leave it unmerged, and I'll have a look as time permits.

Since I started it in gerrit, unless you find the time to do it, leave it on me.

@hjmjohnson hjmjohnson changed the title ENH: Improve itkEuclideanDistancePointMetric coverage. WIP: Improve itkEuclideanDistancePointMetric coverage. Nov 8, 2018
Exercise basic object methods.

Exercise all Set/Get macros.

Refactor the test to accept input parameters to test all possible
conditions.
@jhlegarreta jhlegarreta force-pushed the ImproveEuclideanDistancePointMetricCoverage branch from 3ddf00a to 49bc2e3 Compare November 10, 2018 03:48
@jhlegarreta
Copy link
Member

jhlegarreta commented Nov 10, 2018

@hjmjohnson I've seen that you worked on this. Thanks. I just did a few more changes (used typename for the template arguments and used std::stoi) and rebased on master.

Let's merge it for keeping things moving forward, and some time in the future, when I am in a position to come back to the code coverage issues, I'll check the necessary additions to do.

Thanks for your patience.

@hjmjohnson hjmjohnson changed the title WIP: Improve itkEuclideanDistancePointMetric coverage. ENH: Improve itkEuclideanDistancePointMetric coverage. Nov 10, 2018
@hjmjohnson hjmjohnson merged commit b8e98b8 into InsightSoftwareConsortium:master Nov 10, 2018
@hjmjohnson
Copy link
Member Author

@jhlegarreta Merging based on your comment.

@jhlegarreta
Copy link
Member

Thanks @hjmjohnson. Moving forward !

@hjmjohnson hjmjohnson deleted the ImproveEuclideanDistancePointMetricCoverage branch October 23, 2019 13:28
@jhlegarreta
Copy link
Member

Not seeing the CIs right now or where they were failing/how InsightSoftwareConsortium/ITKExamples/#149 would fix them, but thanks @thewtex .

I guess clang formatting will need to be applied as well.

@dzenanz
Copy link
Member

dzenanz commented Dec 2, 2019

It looks like this PR was merged a year ago. InsightSoftwareConsortium/ITKSphinxExamples#149 adds an example, and does not attempt to fix anything. If you wanted to make follow-up improvements to this, a new PR is in order. Or am I misunderstanding something?

@jhlegarreta
Copy link
Member

Sorry @dzenanz. My bad. Since it was being cross-ref'ed and given that I did not pay due attention to the status (merged vs. open), it looks like I was confused.

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.

4 participants