Skip to content

Closes #913 Refactor xASL_im_ResampleLinearFair #922

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
Merged
MichaelStritt merged 8 commits into from
Nov 29, 2021
Merged

Closes #913 Refactor xASL_im_ResampleLinearFair #922

MichaelStritt merged 8 commits into from
Nov 29, 2021

Conversation

@MichaelStritt
Copy link
Contributor

@MichaelStritt MichaelStritt commented Nov 15, 2021

Linked issue

Check out the issue description in #913.

How to test

If you run ExploreASL the function will already be used in the visualization settings e.g., but there are of course a lot of more refined test options. Loading a NIFTI and resampling it would be a good idea.

@MichaelStritt MichaelStritt linked an issue Nov 15, 2021 that may be closed by this pull request
xASL_im_ResampleLinearFair: simplify code #913
Closed
@MichaelStritt MichaelStritt added the optimization Ensure that code runs faster with unchanged functionality label Nov 15, 2021
@MichaelStritt MichaelStritt self-assigned this Nov 15, 2021
@MichaelStritt

This comment has been minimized.

Sign in to view
jan-petr
jan-petr requested changes Nov 16, 2021
View reviewed changes
Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

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

First - this will work for 1D and 3D changes, but resampling in 2D will be skipped. I fixed that. But this function simply has to be replaced by interp3+ngrid instead of having this enormous thing...

@jan-petr

This comment has been minimized.

Sign in to view
@MichaelStritt
Copy link
Contributor Author

MichaelStritt commented Nov 17, 2021

@jan-petr

First - this will work for 1D and 3D changes, but resampling in 2D will be skipped. I fixed that. But this function simply has to be replaced by interp3+ngrid instead of having this enormous thing...

Correct, but that didn't work before either, right?

HenkMutsaerts
HenkMutsaerts requested changes Nov 18, 2021
View reviewed changes
@HenkMutsaerts

This comment has been minimized.

Sign in to view
@jan-petr

This comment has been minimized.

Sign in to view
jan-petr
jan-petr requested changes Nov 19, 2021
View reviewed changes
Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

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

We should be careful about not dividing by zero when one of the dimension is 1. So mainly input size or new size [X Y 1] should flip it to a 2D mode instead of a 3D mode. Inner dimensions being 1 is a problem as well, but no nice solution - perhaps there we simply calculate the Xq like a mean coordinate. Do you know what I mean?!

@jan-petr

This comment has been minimized.

Sign in to view
@MichaelStritt

This comment has been minimized.

Sign in to view
HenkMutsaerts
HenkMutsaerts requested changes Nov 21, 2021
View reviewed changes
@HenkMutsaerts

This comment has been minimized.

Sign in to view
@MichaelStritt MichaelStritt changed the title Refactor xASL_im_ResampleLinearFair Closes #913 Refactor xASL_im_ResampleLinearFair Nov 26, 2021
HenkMutsaerts
HenkMutsaerts requested changes Nov 26, 2021
View reviewed changes
Copy link
Member

@HenkMutsaerts HenkMutsaerts left a comment

Choose a reason for hiding this comment

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

I discussed this with Jan.

We revert the commits on xASL_im_ResampleLinearFair, and git mv this old function to the development folder, as we may use this later to improve motion correction in high resolution. (it might be faster to do this in a new branch to keep history clean)

Good idea then to create this new function called ResampleLinear and use this instead of ResampleLinearFair.

Again, nice work!

@MichaelStritt MichaelStritt force-pushed the optimization-#913_LinearFair branch from 192187e to c3ee8a8 Compare November 26, 2021 13:00
@MichaelStritt
Copy link
Contributor Author

MichaelStritt commented Nov 26, 2021

I discussed this with Jan.

We revert the commits on xASL_im_ResampleLinearFair, and git mv this old function to the development folder, as we may use this later to improve motion correction in high resolution. (it might be faster to do this in a new branch to keep history clean)

Good idea then to create this new function called ResampleLinear and use this instead of ResampleLinearFair.

Again, nice work!

Sounds good, I cleaned it up. Should have nice individual commits for each file now and it's less convoluted.

jan-petr
jan-petr approved these changes Nov 29, 2021
View reviewed changes
Copy link
Contributor

@jan-petr jan-petr left a comment

Choose a reason for hiding this comment

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

OK.

@MichaelStritt MichaelStritt force-pushed the optimization-#913_LinearFair branch from 5fb66e6 to e20326b Compare November 29, 2021 11:11
@MichaelStritt MichaelStritt merged commit e20326b into develop Nov 29, 2021
@MichaelStritt MichaelStritt deleted the optimization-#913_LinearFair branch November 29, 2021 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jan-petr jan-petr jan-petr approved these changes

@HenkMutsaerts HenkMutsaerts Awaiting requested review from HenkMutsaerts

Assignees

@MichaelStritt MichaelStritt

Labels

optimization Ensure that code runs faster with unchanged functionality

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

xASL_im_ResampleLinearFair: simplify code

4 participants

@MichaelStritt @jan-petr @HenkMutsaerts