Skip to content

Optimize #602 remove b clone2 source #617

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
jan-petr merged 5 commits into from
Jun 4, 2021
Merged

Optimize #602 remove b clone2 source #617

jan-petr merged 5 commits into from
Jun 4, 2021

Conversation

@jan-petr
Copy link
Contributor

@jan-petr jan-petr commented Jun 1, 2021

Linked issue

#602

How to test

Test by running flavors - enough to run DCM2BIDS - further testing not needed and might work only after merging all the other current branches that all work with the import.

@jan-petr jan-petr self-assigned this Jun 1, 2021
@jan-petr jan-petr linked an issue Jun 1, 2021 that may be closed by this pull request
Remove the bClone2Source option in Import #602
Closed
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.

Seems fine to me 👍

@MichaelStritt
Copy link
Contributor

MichaelStritt commented Jun 1, 2021

Tests

I tested using two GE flavors, everything seems fine in the comparison step of the BIDS full pipeline:

==============================================================================================
 ________                      __                                 ______    ______   __        
/        |                    /  |                               /      \  /      \ /  |      
########/  __    __   ______  ## |  ______    ______    ______  /######  |/######  |## |      
## |__    /  \  /  | /      \ ## | /      \  /      \  /      \ ## |__## |## \__##/ ## |      
##    |   ##  \/##/ /######  |## |/######  |/######  |/######  |##    ## |##      \ ## |      
#####/     ##  ##<  ## |  ## |## |## |  ## |## |  ##/ ##    ## |######## | ######  |## |      
## |_____  /####  \ ## |__## |## |## \__## |## |      ########/ ## |  ## |/  \__## |## |_____ 
##       |/##/ ##  |##    ##/ ## |##    ##/ ## |      ##       |## |  ## |##    ##/ ##       |
########/ ##/   ##/ #######/  ##/  ######/  ##/        #######/ ##/   ##/  ######/  ########/ 
                    ## |                                                                      
                    ## |                                                                      
                    ##/  

==================================== ExploreASL Settings =====================================
Dataset Root        
Import Modules      
Process Modules     
bPause              False
iWorker             1
nWorkers            1
==============================================================================================
ExploreASL v1.7.0_BETA initialized ... 
Dataset: GE_PCASL_3Dspiral_Product_pharma
Dataset: GE_PCASL_3Dspiral_volunteer

HenkMutsaerts
HenkMutsaerts requested changes Jun 1, 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.

Few minor issues but looks good overall!

@MichaelStritt MichaelStritt added the optimization Ensure that code runs faster with unchanged functionality label Jun 1, 2021
HenkMutsaerts
HenkMutsaerts requested changes Jun 3, 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.

Cannot find the fallback variable anymore, but where I see the word fallback this should be default instead right? Just to make it understandable. Comments should explain what is happening, not only from a debugger's perspective :)

So let's just pay a bit more attention to commenting, otherwise this thing is really going to bite our back soon. Otherwise it looks good!

@jan-petr
Copy link
Contributor Author

jan-petr commented Jun 3, 2021

Cannot find the fallback variable anymore, but where I see the word fallback this should be default instead right? Just to make it understandable. Comments should explain what is happening, not only from a debugger's perspective :)

So let's just pay a bit more attention to commenting, otherwise this thing is really going to bite our back soon. Otherwise it looks good!

I have removed fallbacks completely :) Because the default value was "", so I just reorganized the script a bit that we don't even need those.

@jan-petr jan-petr requested a review from HenkMutsaerts June 3, 2021 10:55
@HenkMutsaerts
Copy link
Member

HenkMutsaerts commented Jun 3, 2021

Cannot find the fallback variable anymore, but where I see the word fallback this should be default instead right? Just to make it understandable. Comments should explain what is happening, not only from a debugger's perspective :)
So let's just pay a bit more attention to commenting, otherwise this thing is really going to bite our back soon. Otherwise it looks good!

I have removed fallbacks completely :) Because the default value was "", so I just reorganized the script a bit that we don't even need those.

It said "fallback" somewhere in the comment ;) If you change that to "default" then I am happy right?

@jan-petr
Copy link
Contributor Author

jan-petr commented Jun 3, 2021

It said "fallback" somewhere in the comment ;) If you change that to "default" then I am happy right?

Yes exactly. Actually the 'fallback'/'default' setting is now completely removed. As we directly print stuff, without even setting the variable to something - so that comment was implemented and no more relevant.

Read to approve?!

@jan-petr jan-petr requested review from HenkMutsaerts and removed request for HenkMutsaerts June 4, 2021 09:01
@jan-petr jan-petr force-pushed the optimize-#602_RemoveBClone2Source branch from 193c1aa to a11b7c8 Compare June 4, 2021 14:33
@jan-petr jan-petr merged commit a11b7c8 into develop Jun 4, 2021
@jan-petr jan-petr deleted the optimize-#602_RemoveBClone2Source branch June 4, 2021 14:34
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

+1 more reviewer

@MichaelStritt MichaelStritt MichaelStritt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jan-petr jan-petr

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.

Remove the bClone2Source option in Import

4 participants

@jan-petr @MichaelStritt @HenkMutsaerts