Skip to content

#586 xASL_adm_GetPopulationSessions: avoid crashing if no sessions found#587

Merged
BeatrizPadrela merged 6 commits intodevelopfrom
bug-#586_xASL_adm_GetPopulationSessions
Jun 1, 2021
Merged

#586 xASL_adm_GetPopulationSessions: avoid crashing if no sessions found#587
BeatrizPadrela merged 6 commits intodevelopfrom
bug-#586_xASL_adm_GetPopulationSessions

Conversation

@HenkMutsaerts
Copy link
Member

@HenkMutsaerts HenkMutsaerts commented May 16, 2021

Linked issue

Closes #586

@HenkMutsaerts HenkMutsaerts linked an issue May 16, 2021 that may be closed by this pull request
xASL_adm_GetPopulationSessions #586
Closed
MDijsselhof
MDijsselhof reviewed May 25, 2021
View reviewed changes
MDijsselhof
MDijsselhof reviewed May 25, 2021
View reviewed changes
Copy link
Contributor

@MDijsselhof MDijsselhof left a comment

Choose a reason for hiding this comment

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

Hi Henk-Jan,

I've left a single comment, curious what you think!

@MichaelStritt MichaelStritt added the bug Something isn't working label May 31, 2021
jan-petr
jan-petr requested changes May 31, 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 have to update certain fields similarly as done in #480 . Becausehere, we add new x.nSessions and x.nSubjectSessions - the whole difference file has to be checked and potentially updated... This won't show as a conflict as these are newly added lines. @MichaelStritt should check as he knows what to fix...

@MichaelStritt
Copy link
Contributor

MichaelStritt commented May 31, 2021

We have to update certain fields similarly as done in #480 . Becausehere, we add new x.nSessions and x.nSubjectSessions - the whole difference file has to be checked and potentially updated... This won't show as a conflict as these are newly added lines. @MichaelStritt should check as he knows what to fix...

In Functions/xASL_stat_GetROIstatistics.m:

  • we moved x.WBmask to x.S.masks.WBmask

In Functions/xASL_stat_PrintStats.m:

  • we moved x.nSessions to x.dataset.nSessions

@jan-petr jan-petr requested a review from MichaelStritt June 1, 2021 08:35
@MichaelStritt
Copy link
Contributor

MichaelStritt commented Jun 1, 2021

@jan-petr: I only changed the code sections that were changed by #586. The should work with #480 now. There will still be some conflict resolving.

@MichaelStritt MichaelStritt requested a review from jan-petr June 1, 2021 09:36
MichaelStritt
MichaelStritt approved these changes Jun 1, 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.

Should be fine with regard to #480

HenkMutsaerts
HenkMutsaerts commented Jun 1, 2021
View reviewed changes
Copy link
Member Author

@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.

Am I actually reviewing my own work? :) I thought I found some parts that I wrote :)

@HenkMutsaerts
Copy link
Member Author

HenkMutsaerts commented Jun 1, 2021

@jan-petr @MDijsselhof Yes, I agree. We might soon revamp all this when we move to BIDS derivatives, but for now I believe it is safe to assume that there is an ASL_1. If there are no ASL sessions, this works as well.

@MichaelStritt
Copy link
Contributor

MichaelStritt commented Jun 1, 2021

Great, then only @jan-petr needs to re-approve

@jan-petr jan-petr requested a review from MDijsselhof June 1, 2021 13:35
jan-petr
jan-petr approved these changes Jun 1, 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.

Fine to merge.

@jan-petr jan-petr requested review from MDijsselhof and removed request for BeatrizPadrela and MDijsselhof June 1, 2021 13:35
@jan-petr
Copy link
Contributor

jan-petr commented Jun 1, 2021

I am not able to remove Mathijs from the review - he either appears as not-approved-yet or as has-a-comment. So lets keep it as it is and merge. @BeatrizPadrela seems to be assigned for this ;)

@BeatrizPadrela BeatrizPadrela force-pushed the bug-#586_xASL_adm_GetPopulationSessions branch from 4edd87b to 1df4968 Compare June 1, 2021 14:01
@BeatrizPadrela BeatrizPadrela merged commit 1df4968 into develop Jun 1, 2021
@BeatrizPadrela BeatrizPadrela deleted the bug-#586_xASL_adm_GetPopulationSessions branch June 1, 2021 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MDijsselhof MDijsselhof MDijsselhof left review comments

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

+1 more reviewer

@MichaelStritt MichaelStritt MichaelStritt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@BeatrizPadrela BeatrizPadrela

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

xASL_adm_GetPopulationSessions

5 participants

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