Skip to content

Fixes #759 Adapting xASL to multi-PLD #871

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
MDijsselhof merged 11 commits into from
Nov 19, 2021
Merged

Fixes #759 Adapting xASL to multi-PLD #871

MDijsselhof merged 11 commits into from
Nov 19, 2021

Conversation

@MDijsselhof
Copy link
Contributor

@MDijsselhof MDijsselhof commented Oct 4, 2021

Linked issue

closes #759

How to test

Required: if not defined in the linked issue, add a simple test description here

Comments

Optional: add helpful comments for the reviewers here

@MDijsselhof MDijsselhof linked an issue Oct 4, 2021 that may be closed by this pull request
Adapting ExploreASL pipeline to multi-PLD data #759
Closed
@MDijsselhof MDijsselhof requested a review from jan-petr October 4, 2021 15:54
@MichaelStritt MichaelStritt changed the title Feature #759 adaptingx asl to multi pld Fixes #759 Adaptingx ASL to multi-PLD Oct 4, 2021
@MichaelStritt MichaelStritt changed the title Fixes #759 Adaptingx ASL to multi-PLD Fixes #759 Adapting xASL to multi-PLD Oct 4, 2021
@MichaelStritt MichaelStritt added the feature New feature, enhancement or request label Oct 4, 2021
MichaelStritt
MichaelStritt reviewed Oct 4, 2021
View reviewed changes
MichaelStritt
MichaelStritt reviewed Oct 4, 2021
View reviewed changes
@MichaelStritt MichaelStritt self-requested a review October 4, 2021 16:45
MichaelStritt
MichaelStritt approved these changes Oct 4, 2021
View reviewed changes
Copy link
Contributor

@MichaelStritt MichaelStritt left a comment

Choose a reason for hiding this comment

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

I only found minor stuff. Please check my comments and wait for @jan-petr's review. Nice work 👍

jan-petr
jan-petr requested changes Oct 4, 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.

Very good - almost there. Most of my comments are really minor to improve the code quality. But there are 1-2 bugs and 1-2 important things to discuss.

@MichaelStritt MichaelStritt requested a review from jan-petr October 4, 2021 19:24
HenkMutsaerts
HenkMutsaerts requested changes Oct 5, 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.

Sorry this is a big change so will get lots of comments. Hoge bomen vangen veel wind ;)

jan-petr
jan-petr requested changes Oct 5, 2021
View reviewed changes
@MDijsselhof
Copy link
Contributor Author

MDijsselhof commented Oct 5, 2021

@jan-petr Apparently I can't comment if someone requests changes so I'll just write it down here.
Regarding the x.Q.bUseBasilQuantification: is this fine the way it is now? I don't follow.
Regarding the x struct: this was required to output qCBF_Basil.nii and relatable paths, however if we change all paths back to the original qCBF.nii this is not required anymore. Does that work for you?

jan-petr
jan-petr approved these changes Oct 12, 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.

All good now.

@MDijsselhof MDijsselhof force-pushed the feature-#759_AdaptingxASLToMultiPLD branch from 06389af to dae7460 Compare November 19, 2021 10:46
MichaelStritt and others added 4 commits November 19, 2021 12:07
@MichaelStritt @MDijsselhof
#759 Minor optimizations
874cd80 
I tried to help with some of Jans review comments. Still not everything is done. Cheers, Michael.
@MDijsselhof
#759 review requested changes
8ef03b5
@BeatrizPadrela @MDijsselhof
#759 Default conditions - minor changes
1eec167
@jan-petr @MDijsselhof
#759 Minor optimization
12cc7d6
@MDijsselhof MDijsselhof force-pushed the feature-#759_AdaptingxASLToMultiPLD branch from dae7460 to 12cc7d6 Compare November 19, 2021 11:08
@MDijsselhof MDijsselhof merged commit 12cc7d6 into develop Nov 19, 2021
@MichaelStritt MichaelStritt deleted the feature-#759_AdaptingxASLToMultiPLD branch November 19, 2021 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HenkMutsaerts HenkMutsaerts HenkMutsaerts approved these changes

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

@BeatrizPadrela BeatrizPadrela BeatrizPadrela approved these changes

+1 more reviewer

@MichaelStritt MichaelStritt MichaelStritt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@MDijsselhof MDijsselhof

Labels

feature New feature, enhancement or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Adapting ExploreASL pipeline to multi-PLD data

6 participants

@MDijsselhof @HenkMutsaerts @jan-petr @MichaelStritt @BeatrizPadrela