ADD: Addition of point to surface metric to elastix. #236

gokhangg · 2020-04-04T12:34:06Z

No description provided.

N-Dekker · 2020-04-07T09:37:35Z

Thank you Gokhan @gokhangg I just retrieved your work by cloning github.com/gokhangg/elastix.git and did an initial build. Everything builds fine, also on my laptop. 😃 However, it appears that your work is not included with the compilation! Do you still have a CMakeLists.txt that should be added, or a modification to an existing elastix CMake file?

gokhangg · 2020-04-07T14:31:09Z

"However, it appears that your work is not included with the compilation! Do you still have a CMakeLists.txt that should be added, or a modification to an existing elastix CMake file?"

Could not get here, could you please clarify?

N-Dekker · 2020-04-07T15:21:21Z

Could not get here, could you please clarify?

Your "elxPointToSurfaceDistanceMetric.cxx"should be compiled when elastix is built, right? Does it actually get compiled on your computer? When you deliberately make a syntax error in your cxx file, for example by typing #error Please get me a compilation error!, do you get a compilation error indeed?

N-Dekker · 2020-04-08T12:45:02Z

I think you should add a "CMakeLists.txt" to the directory of your metric, having something like this:

ADD_ELXCOMPONENT( PointToSurfaceDistanceMetric ON
  elxPointToSurfaceDistanceMetric.h
  elxPointToSurfaceDistanceMetric.hxx
  elxPointToSurfaceDistanceMetric.cxx
  itkPointToSurfaceDistanceMetric.h
  itkPointToSurfaceDistanceMetric.hxx )

Also I think the directory of your metric should be renamed to "PointToSurfaceDistanceMetric" (by adding the word "Metric"), otherwise I still get build errors.

gokhangg · 2020-04-08T13:22:41Z

Hi Niels, I have checked this issue and it seems that the .gitignore blocks uploading cmake related files.
Line 36 ignores "cmake*". So actually that file was inside but git ignored it while pushing. So I think you should delete that line in the gitignore file or I can send that CMakeLists.txt file to you and then you can upload to the repo.

N-Dekker · 2020-04-08T13:45:02Z

@gokhangg

I have checked this issue and it seems that the .gitignore blocks uploading cmake related files.
Line 36 ignores "cmake*".

Wow, well spotted! I'll remove "cmake*" from the ignore-list, thanks!

Having cmake* in there stopped Gokhan Gunay from adding his CMakeLists file to pull request #236 (Addition of PointToSurface metric).

N-Dekker · 2020-04-08T14:03:59Z

@gokhangg Could you please still try to add your CMakeLists to your branch, using the git option "--force"? Something like this, if I understand correctly:

git add --force CMakeLists.txt

As I read at https://stackoverflow.com/questions/24844782/how-to-override-a-rule-set-in-gitignore

gokhangg · 2020-04-08T14:35:43Z

Sure I can indeed bypass the ignore but just wanted to inform you. After the pull for the cmake fix, could you inform me then I will update my request?

Having cmake* in there stopped Gokhan Gunay from adding his CMakeLists file to pull request #236 (Addition of PointToSurface metric).

N-Dekker · 2020-04-08T14:59:12Z

OK, I removed "cmake*" from .gitignore (pull request #239). Can you please add your CMakeLists.txt to your pull request?

gokhangg · 2020-04-08T16:09:14Z

Sure

gokhangg · 2020-04-08T16:25:25Z

You can merge now ;)

Components/Metrics/PointToSurfaceDistance/elxPointToSurfaceDistanceMetric.h

Components/Metrics/PointToSurfaceDistance/elxPointToSurfaceDistanceMetric.cxx

gokhangg · 2020-04-09T09:21:51Z

Can we copy from other files? I think should be the same ??

Components/Metrics/PointToSurfaceDistance/elxPointToSurfaceDistanceMetric.h

Components/Metrics/PointToSurfaceDistance/itkPointToSurfaceDistanceMetric.h

Components/Metrics/PointToSurfaceDistance/elxPointToSurfaceDistanceMetric.h

N-Dekker · 2020-04-14T12:30:10Z

Components/Metrics/PointToSurfaceDistance/itkPointToSurfaceDistanceMetric.h

+    /** Run-time type information (and related methods). */
+    itkTypeMacro( PointToSurfaceDistanceMetric, SingleValuedPointSetToPointSetMetric );//OK
+    /**  Initialize. */
+    virtual void Initialize() noexcept(false);


I see you marked some member functions noexcept(false), indicating that they might throw an exception. Why would you do so? When I use noexcept, it's usually just to indicate that a function does not throw.

@gokhangg Thanks for your patience. I still see noexcept(false) five times in your proposal. Would you want to remove these as well? Or do they have a special reason?

All removed

Thanks @gokhangg Nowadays I think it's better to only use noexcept when the benefit is really clear (which is clearly the case for move-constructors and move-assignment operators). But other people think otherwise... For your information, I did a little lightning talk about noexcept at the conference C++ on Sea, two years ago https://www.youtube.com/watch?v=dVRLp-Rwg0k regarding my noexcept benchmark

gokhangg · 2021-02-05T11:40:04Z

No description provided.

Can you please add a description to your commit? Actually that's a request from Marius (@mstaring). 😃 The formatting convention for a commit message is as follows:

The first line of the commit message starts with "ENH: " for enhancement, and is at most 72 characters (ITK convention). It is the "subject line" of the commit message.

The second line is entirely empty.

The next lines give a more detailed description.

gokhangg · 2021-02-05T11:46:03Z

can you comment on this PR? Is it ready for merging?

Hi Marius @mstaring I gave a comment, is it possible to check if it is in the correct condition?

N-Dekker · 2021-02-05T12:46:40Z

@gokhangg Can you possibly squash all (four) commits of your pull request together, to make it one single commit?

 git rebase -i HEAD~4

An then replace "pick" on the second and subsequent commits with "squash": https://stackoverflow.com/questions/5189560/squash-my-last-x-commits-together-using-git

And then git push --force-with-lease, of course 😃

Replaced `_ELASTIX_BUILD_LIBRARY` preprocessor definition by static member function, `BaseComponent::IsElastixLibrary()`. Anticipates sharing the intermediate binary object files between the Elastix executable and the Elastix library, possibly building them within the same project, or the same (Visual Studio) solution. Addition of PointToSurface metric COMP: Remove cmake* from .gitignore Having cmake* in there stopped Gokhan Gunay from adding his CMakeLists file to pull request SuperElastix#236 (Addition of PointToSurface metric). Addition of PointToSurface metric

gokhangg · 2021-02-05T13:20:30Z

@N-Dekker Hi Niels there are some conflicts popped up after squashing the middle-commits. So seems that there is a discrepancy between my fork and the superelastix::develop branch. So shall we resolve the conflicts manually?

COMP: Remove cmake* from .gitignore Having cmake* in there stopped Gokhan Gunay from adding his CMakeLists file to pull request SuperElastix#236 (Addition of PointToSurface metric). Addition of PointToSurface metric ADD: Addition of point to surface metric to Elastix. In this commit addition of point to surface metric is done. This metric essentially considers how surface of an masked organ is aligned with the points provided from the surface of the organ. Therefore, it is used to improve alignment of the organ under interest.

N-Dekker · 2021-02-05T14:01:14Z

Oh no... git happening 😢 Should any of those conflicting files be adjusted by your pull request? I mean, did you intend to propose changes to any of those files?

N-Dekker · 2021-02-05T14:11:13Z

@gokhangg It would be wonderful if you could fix the git conflicts. But you probably need a lot of "git magic" to get it right. It might be easier to just create a new branch from the latest SuperElastix/elastix "develop", and create a new pull request. (Don't feel bad... git happens 😸) I leave it up to you.

gokhangg · 2021-02-05T14:19:31Z

@N-Dekker ACtually those changed files are coming from other places not in this PR. I will try to solve it.

Having cmake* in there stopped Gokhan Gunay from adding his CMakeLists file to pull request SuperElastix#236 (Addition of PointToSurface metric).

COMP: Remove cmake* from .gitignore Having cmake* in there stopped Gokhan Gunay from adding his CMakeLists file to pull request SuperElastix#236 (Addition of PointToSurface metric). Addition of PointToSurface metric

Replaced `_ELASTIX_BUILD_LIBRARY` preprocessor definition by static member function, `BaseComponent::IsElastixLibrary()`. Anticipates sharing the intermediate binary object files between the Elastix executable and the Elastix library, possibly building them within the same project, or the same (Visual Studio) solution. Addition of PointToSurface metric COMP: Remove cmake* from .gitignore Having cmake* in there stopped Gokhan Gunay from adding his CMakeLists file to pull request SuperElastix#236 (Addition of PointToSurface metric). Addition of PointToSurface metric

COMP: Remove cmake* from .gitignore Having cmake* in there stopped Gokhan Gunay from adding his CMakeLists file to pull request SuperElastix#236 (Addition of PointToSurface metric). Addition of PointToSurface metric ADD: Addition of point to surface metric to Elastix. In this commit addition of point to surface metric is done. This metric essentially considers how surface of an masked organ is aligned with the points provided from the surface of the organ. Therefore, it is used to improve alignment of the organ under interest.

gokhangg · 2021-02-05T14:27:27Z

@N-Dekker I have found out that it is related to neither the changes I did nor the discrepancy in the base. There was a nonlinearity in the commit line so after squashing all other commits except the one breaking the linearity it was solved. Indeed I changed the message of the commit :)

N-Dekker · 2021-02-05T15:27:05Z

Components/Metrics/PointToSurfaceDistance/itkPointToSurfaceDistanceMetric.hxx

+::Initialize()
+{
+  /** Initialize transform, interpolator, etc.
+  Superclass::Initialize();


Is it the intention that this Superclass::Initialize() call is commented out?

N-Dekker · 2021-02-05T15:34:54Z

Components/Metrics/PointToSurfaceDistance/elxPointToSurfaceDistanceMetric.h

+ *
+ *=========================================================================*/
+#ifndef __elxPointToSurfaceDistanceMetric_H__
+#define __elxPointToSurfaceDistanceMetric_H__


(Minor nitpick) In elastix, as well as in ITK, we no longer have those __ underscores at the begin and the end of include guard macro names. See #339 Of course, we can remove them from your header files later, no problem.

N-Dekker · 2021-02-08T13:04:06Z

@gokhangg Could you possibly also include a little test (unit test) with your pull request?

N-Dekker added a commit that referenced this pull request Apr 8, 2020

COMP: Remove cmake* from .gitignore

12134c2

Having cmake* in there stopped Gokhan Gunay from adding his CMakeLists file to pull request #236 (Addition of PointToSurface metric).

N-Dekker mentioned this pull request Apr 8, 2020

COMP: Remove cmake* from .gitignore #239

Merged

N-Dekker added a commit that referenced this pull request Apr 8, 2020

COMP: Remove cmake* from .gitignore

350ff50

Having cmake* in there stopped Gokhan Gunay from adding his CMakeLists file to pull request #236 (Addition of PointToSurface metric).

gokhangg force-pushed the develop branch 2 times, most recently from b8befdd to 0875d28 Compare April 8, 2020 16:24