search

CBICA / CaPTk

Cancer Imaging Phenomics Toolkit (CaPTk) is a software platform to perform image analysis and predictive modeling tasks. Documentation: https://cbica.github.io/CaPTk
https://www.cbica.upenn.edu/captk
Other
175 stars 63 forks source link

1369 #1372

Closed sarthakpati closed 3 years ago

sarthakpati commented 3 years ago

Fixes issue #1369

Proposed Changes

sarthakpati commented 3 years ago

Looks great. Just a minor change - Can we move the function to cpp file?

I was initially trying to use the implementation that you had (basically, the function in CPP). But, the standalone applications were not getting linked properly and I unfortunately don't have time to debug this. Please feel free to have a crack it if you want but if you don't have the time either, I would suggest please merge.

ashishsingh18 commented 3 years ago

Looks great. Just a minor change - Can we move the function to cpp file?

I was initially trying to use the implementation that you had (basically, the function in CPP). But, the standalone applications were not getting linked properly and I unfortunately don't have time to debug this. Please feel free to have a crack it if you want but if you don't have the time either, I would suggest please merge.

This is very simple. Just move the implementation to cpp file. I fixed it and created a PR to your branch.

sarthakpati commented 3 years ago

Looks great. Just a minor change - Can we move the function to cpp file?

I was initially trying to use the implementation that you had (basically, the function in CPP). But, the standalone applications were not getting linked properly and I unfortunately don't have time to debug this. Please feel free to have a crack it if you want but if you don't have the time either, I would suggest please merge.

This is very simple. Just move the implementation to cpp file. I fixed it and created a PR to your branch.

Ashish, I know how to move something to the CPP file. The issue is that it causes link errors.

ashishsingh18 commented 3 years ago

Looks great. Just a minor change - Can we move the function to cpp file?

I was initially trying to use the implementation that you had (basically, the function in CPP). But, the standalone applications were not getting linked properly and I unfortunately don't have time to debug this. Please feel free to have a crack it if you want but if you don't have the time either, I would suggest please merge.

This is very simple. Just move the implementation to cpp file. I fixed it and created a PR to your branch.

Ashish, I know how to move something to the CPP file. The issue is that it causes link errors.

I am not getting any linker errors after moving to cpp.

sarthakpati commented 3 years ago

Looks great. Just a minor change - Can we move the function to cpp file?

I was initially trying to use the implementation that you had (basically, the function in CPP). But, the standalone applications were not getting linked properly and I unfortunately don't have time to debug this. Please feel free to have a crack it if you want but if you don't have the time either, I would suggest please merge.

This is very simple. Just move the implementation to cpp file. I fixed it and created a PR to your branch.

Ashish, I know how to move something to the CPP file. The issue is that it causes link errors.

I am not getting any linker errors after moving to cpp.

Great! Have you merged from this change that I am proposing? That is, are you actually using that function?

ashishsingh18 commented 3 years ago

Looks great. Just a minor change - Can we move the function to cpp file?

I was initially trying to use the implementation that you had (basically, the function in CPP). But, the standalone applications were not getting linked properly and I unfortunately don't have time to debug this. Please feel free to have a crack it if you want but if you don't have the time either, I would suggest please merge.

This is very simple. Just move the implementation to cpp file. I fixed it and created a PR to your branch.

Ashish, I know how to move something to the CPP file. The issue is that it causes link errors.

I am not getting any linker errors after moving to cpp.

Great! Have you merged from this change that I am proposing? That is, are you actually using that function?

Have you tried the changes I proposed and created the PR? I opened that PR only after testing it. That works!

sarthakpati commented 3 years ago

Looks great. Just a minor change - Can we move the function to cpp file?

I was initially trying to use the implementation that you had (basically, the function in CPP). But, the standalone applications were not getting linked properly and I unfortunately don't have time to debug this. Please feel free to have a crack it if you want but if you don't have the time either, I would suggest please merge.

This is very simple. Just move the implementation to cpp file. I fixed it and created a PR to your branch.

Ashish, I know how to move something to the CPP file. The issue is that it causes link errors.

I am not getting any linker errors after moving to cpp.

Great! Have you merged from this change that I am proposing? That is, are you actually using that function?

Have you tried the changes I proposed and created the PR? I opened that PR only after testing it. That works!

I just said that it does not work. Here is a screenshot: image

AlexanderGetka-cbica commented 3 years ago

If you're adding the cpp file maybe it just needs a cmake configure to pick it up? If the Azure build succeeds, we'll know it links.

ashishsingh18 commented 3 years ago

Looks great. Just a minor change - Can we move the function to cpp file?

I was initially trying to use the implementation that you had (basically, the function in CPP). But, the standalone applications were not getting linked properly and I unfortunately don't have time to debug this. Please feel free to have a crack it if you want but if you don't have the time either, I would suggest please merge.

This is very simple. Just move the implementation to cpp file. I fixed it and created a PR to your branch.

Ashish, I know how to move something to the CPP file. The issue is that it causes link errors.

I am not getting any linker errors after moving to cpp.

Great! Have you merged from this change that I am proposing? That is, are you actually using that function?

Have you tried the changes I proposed and created the PR? I opened that PR only after testing it. That works!

I just said that it does not work. Here is a screenshot: image

It works perfectly on my side. image

Why don't you let it run on Azure?

sarthakpati commented 3 years ago

If you're adding the cpp file maybe it just needs a cmake configure to pick it up? If the Azure build succeeds, we'll know it links.

Well, it flunked out on my local machine. If I know something fails locally, I am obviously not going to ask Azure to go through it.

sarthakpati commented 3 years ago

Looks great. Just a minor change - Can we move the function to cpp file?

I was initially trying to use the implementation that you had (basically, the function in CPP). But, the standalone applications were not getting linked properly and I unfortunately don't have time to debug this. Please feel free to have a crack it if you want but if you don't have the time either, I would suggest please merge.

This is very simple. Just move the implementation to cpp file. I fixed it and created a PR to your branch.

Ashish, I know how to move something to the CPP file. The issue is that it causes link errors.

I am not getting any linker errors after moving to cpp.

Great! Have you merged from this change that I am proposing? That is, are you actually using that function?

Have you tried the changes I proposed and created the PR? I opened that PR only after testing it. That works!

I just said that it does not work. Here is a screenshot: image

It works perfectly on my side. image

Why don't you let it run on Azure?

Great, then why don't you put it as a separate PR and I can close this? I don't want to take responsibility for that code, at all.

ashishsingh18 commented 3 years ago

Looks great. Just a minor change - Can we move the function to cpp file?

I was initially trying to use the implementation that you had (basically, the function in CPP). But, the standalone applications were not getting linked properly and I unfortunately don't have time to debug this. Please feel free to have a crack it if you want but if you don't have the time either, I would suggest please merge.

This is very simple. Just move the implementation to cpp file. I fixed it and created a PR to your branch.

Ashish, I know how to move something to the CPP file. The issue is that it causes link errors.

I am not getting any linker errors after moving to cpp.

Great! Have you merged from this change that I am proposing? That is, are you actually using that function?

Have you tried the changes I proposed and created the PR? I opened that PR only after testing it. That works!

I just said that it does not work. Here is a screenshot: image

It works perfectly on my side. image Why don't you let it run on Azure?

Great, then why don't you put it as a separate PR and I can close this? I don't want to take responsibility for that code, at all.

It really should go with this PR. I don't understand the responsibility part. This is your code. I just moved it to the cpp file verbatim.

sarthakpati commented 3 years ago

It really should go with this PR. I don't understand the responsibility part. This is your code. I just moved it to the cpp file verbatim.

The point is that it does not work for me. If I cannot generate results with it, I am not going to take responsibility for it. The code that I have works and does what I expect it do but your changes are breaking it on my test bed.