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.

Sign up for GitHub

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

Jump to bottom

1369 #1372

Merged
merged 14 commits into from
Dec 3, 2020
Merged

1369 #1372

merged 14 commits into from
Dec 3, 2020

Conversation

sarthakpati
Copy link
Contributor

Fixes issue #1369

Proposed Changes

  • perfusion alignment plots are getting saved

ashishsingh18
ashishsingh18 requested changes Nov 12, 2020
View reviewed changes
Copy link
Collaborator

@ashishsingh18 ashishsingh18 left a comment

Choose a reason for hiding this comment

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

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

@sarthakpati
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

sarthakpati commented Nov 12, 2020 •
edited
Loading

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.

ashishsingh18
ashishsingh18 previously approved these changes Nov 16, 2020
View reviewed changes
AlexanderGetka-cbica
AlexanderGetka-cbica approved these changes Dec 3, 2020
View reviewed changes
Copy link
Contributor

@AlexanderGetka-cbica AlexanderGetka-cbica left a comment

Choose a reason for hiding this comment

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

Approving based on prior discussion. Adding an enhancement request to change the .cpp/.h divide later once we have more time to double-check.

@AlexanderGetka-cbica AlexanderGetka-cbica mentioned this pull request Dec 3, 2020
Open
@sarthakpati sarthakpati merged commit ad45a5b into CBICA:master Dec 3, 2020
@sarthakpati sarthakpati deleted the 1369 branch December 3, 2020 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexanderGetka-cbica AlexanderGetka-cbica AlexanderGetka-cbica approved these changes

@ashishsingh18 ashishsingh18 Awaiting requested review from ashishsingh18

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sarthakpati @ashishsingh18 @AlexanderGetka-cbica