Revamp #480 restructure #607

MichaelStritt · 2021-05-28T00:10:01Z

Linked issue

Check out #480

How to test

Pray and hope that I broke nothing 👍

Comments

I think this should be enough for now. We can do further restructuring later on 👍

Edit 28.05.2021: I just ran the test dataset on low quality and there everything seems to work. Could be really painful to check all the individual commits, but the edits are pretty straight forward. The only real code changes are here f6f5253 and here bef5bf4 .

jan-petr

Looks like a lot of changes. Great work, though my worries of course dampen my enthusiasm :)

Probably pointless to ask if you always searched for all occurrences to make sure this was replaced everywhere.

If this was done, then I only see a single possibility that you have missed some parameters. And that is if something was changed between the branching and future merging. In these cases, you probably want to scrutinize these commits and make sure that all these are added also to all things modified in between. Note that some can be identified by conflicts, but new code additions won't. That might be some amount of work - what do you think, how much commits were there inbetween?

MichaelStritt · 2021-05-31T10:24:05Z

Looks like a lot of changes. Great work, though my worries of course dampen my enthusiasm :)

Thanks 👍

Probably pointless to ask if you always searched for all occurrences to make sure this was replaced everywhere.

I turned on the indexing on windows to search within files. This way it was pretty easy to search for all occurrences. Searching for x.VARIABLE is pretty easy, but sometimes we check if isfield(x,'VARIABLE'). I tried to catch all of those.

If this was done, then I only see a single possibility that you have missed some parameters. And that is if something was changed between the branching and future merging. In these cases, you probably want to scrutinize these commits and make sure that all these are added also to all things modified in between. Note that some can be identified by conflicts, but new code additions won't. That might be some amount of work - what do you think, how much commits were there inbetween?

The branch was created four days ago and all of it was implemented within the last four days as well. Most of the changes that happened within the last four days within develop were import related, so I don't see a big issue there. It is a really tricky issue though. Letting it grow older makes things worse though :D

There are still a lot of fields that could be improved, but I thought it would be good not to do everything at once. Especially because we develop so much stuff in parallel now.

jan-petr · 2021-05-31T11:51:35Z

turned on the indexing on windows to search within files. This way it was pretty easy to search for all occurrences. Searching for x.VARIABLE is pretty easy, but

Agree with all. Yes - all the latest commits/edits were import related, so hopefully nothing really conflicting. But still - do we want to check to be sure? Or we quickly merge? Has Henk seen it? Better ask now and not receive questions later ;)

MichaelStritt · 2021-05-31T12:19:13Z

turned on the indexing on windows to search within files. This way it was pretty easy to search for all occurrences. Searching for x.VARIABLE is pretty easy, but

Agree with all. Yes - all the latest commits/edits were import related, so hopefully nothing really conflicting. But still - do we want to check to be sure? Or we quickly merge? Has Henk seen it? Better ask now and not receive questions later ;)

Already checked it. The only commits in between were for #600, #603 & #612. The files that were changed are xASL_bids_BIDS2Legacy.m, xASL_imp_BIDS2Legacy.m & ExploreASL_Initialize.m. The only conflict should be in ExploreASL_Initialize.m. There were no changes in the same lines though.
Sure, we can ask @HenkMutsaerts. I'm just not sure if he has time today and it would be really helpful to merge this quickly, because merging other stuff in between is quite problematic.

HenkMutsaerts

Is x.dir the dataset-related folders (currently in x.P)? Or environment/ExploreASL-related folders (currently in x.D)?
Should x.tools be external?
What is the difference between x.opts and x.settings? Input arguments travel through the pipeline right
Also:
WBmask -> belongs to statistics
Skull -> belongs to vis
What is the use for x.utils?
Do you want settings to be separated per module? If yes, then BILAT_FILTER is ASL module
Pediatric_Template is an atlas setting?

MichaelStritt · 2021-05-31T13:08:39Z

Is x.dir the dataset-related folders (currently in x.P)? Or environment/ExploreASL-related folders (currently in x.D)?

I used it for global directories, like the dataset root, the studyPar, sourceStructure etc. and I thought that those rather complex directories related to mutex etc. are better there than in x.P or x.D, especially since those ones get pretty overfilled and unstructured in the end.

Should x.tools be external?

Personal choice, like all of the other fields as well. In the document within the revamp drive and within the ABBA meeting about that we agreed on tools for toolboxes and for me SPM is a toolbox.

What is the difference between x.opts and x.settings? Input arguments travel through the pipeline right

x.opts are only the input options of exploreasl master and it's close derivatives right now. x.settings are general pipeline settings that were in exploreasl before and that were in the main level of the x struct before. I think it's a lot cleaner the way it is now and we can easily improve minor things later on.

WBmask -> belongs to statistics
Skull -> belongs to vis

For me both of those belong to something like x.masks or x.utilities, but sure, if you insist on that. I guess with statistics you mean x.S ?

Do you want settings to be separated per module? If yes, then BILAT_FILTER is ASL module

No, we're not there yet. We need to do this step by step and make sure that the pipeline stays stable. After that we can think about further restructuring.

Pediatric_Template is an atlas setting?

Is it? I only know that it's used in the structural module and defined in the data dependent settings.

HenkMutsaerts · 2021-05-31T14:01:42Z

Is x.dir the dataset-related folders (currently in x.P)? Or environment/ExploreASL-related folders (currently in x.D)?

I used it for global directories, like the dataset root, the studyPar, sourceStructure etc. and I thought that those rather complex directories related to mutex etc. are better there than in x.P or x.D, especially since those ones get pretty overfilled and unstructured in the end.

That sounds like x.D. Then I would replace all x.D with x.dir.

Should x.tools be external?

Personal choice, like all of the other fields as well. In the document within the revamp drive and within the ABBA meeting about that we agreed on tools for toolboxes and for me SPM is a toolbox.

True, though we already have a folder called External. Always helpful to use the same terms, not using a table to go from "folder terminology" to "internal memory terminology" ;)

What is the difference between x.opts and x.settings? Input arguments travel through the pipeline right

x.opts are only the input options of exploreasl master and it's close derivatives right now. x.settings are general pipeline settings that were in exploreasl before and that were in the main level of the x struct before. I think it's a lot cleaner the way it is now and we can easily improve minor things later on.

Sure but can becoming confusing because the original settings can also contain input arguments, like which module to run etc?

WBmask -> belongs to statistics
Skull -> belongs to vis

For me both of those belong to something like x.masks or x.utilities, but sure, if you insist on that. I guess with statistics you mean x.S ?

The masks are used for statistics and visualization. But a good idea to create a x.masks or x.stats.masks! As long as it stays readable :)
I would replace x.S by x.stats for consistency
What is utilities? This sounds a bit too general. How does this differ from settings?
skull is a mask as well, used for visualization, so could also go to x.stats.masks

Do you want settings to be separated per module? If yes, then BILAT_FILTER is ASL module

No, we're not there yet. We need to do this step by step and make sure that the pipeline stays stable. After that we can think about further restructuring.

Pediatric_Template is an atlas setting?

Is it? I only know that it's used in the structural module and defined in the data dependent settings.

True, each template is an atlas but it might be used for a different goal indeed. Here the pediatric atlas/template is used for improving registration of young kids, whereas later in the population module these can be used for improved definition of ROIs

jan-petr · 2021-05-31T15:21:00Z

@MichaelStritt I will let you resolve all the errors by Henk. At the same time, I will run some tests and check the code as well for mistakes. But I will report them in comments to avoid conflicting edits...

MichaelStritt · 2021-05-31T15:25:17Z

That's it for me. I don't think it makes sense to change even more now. There's a follow up issue to improve this later on.

jan-petr

xASL_adm_CleanUpBeforeRerun.m Line 369-375: SPMdir and SPMpath are in a subfield.

jan-petr

Functions/xASL_init_DefineDataDependentSettings.m:39:if ~isfield(x,'Pediatric_Template')

jan-petr

Functions/xASL_adm_CompareDataSets.m:40: if isfield(x,'bReproTesting')
Functions/xASL_init_DefineDataDependentSettings.m:25:if ~isfield(x,'bReproTesting')
Functions/xASL_init_DefineDataDependentSettings.m:30:if isfield(x, 'bReproTesting')
Functions/xASL_Iteration.m:35: if ~isfield(x,'dryRun')
Modules/SubModule_ASL/xASL_wrp_ProcessM0.m:83:if ~isfield(x,'M0_conventionalProcessing')
Modules/xASL_module_Structural.m:347:if ~isfield(x,'SegmentSPM12')
ExploreASL_ProcessMaster.m:46: if isfield(x,'SegmentSPM12')
Functions/xASL_init_DefineDataDependentSettings.m:72:if ~isfield(x, 'SegmentSPM12')
Functions/xASL_init_DefineDataDependentSettings.m:75: x.SegmentSPM12 = x.Segment_SPM12;

jan-petr · 2021-05-31T15:47:56Z

That's it for me. I don't think it makes sense to change even more now. There's a follow up issue to improve this later on.

Agree - the others need a more discussion. And this is all internal, so can be done in two steps. However - please see all the errors I found. They look to me like forgotten renames.

It is mostly isfield(x,'field') instead of isfield(x.subgroup,'field'). And note that in some cases, you might need to add isfield(x,'subgroup'). I keep on searching...

MichaelStritt · 2021-05-31T15:54:58Z

Damn, I thought I found all the isfield ones. They are a bit tricky with the search function. I can fix that 👍

MichaelStritt · 2021-05-31T16:06:24Z

xASL_adm_CleanUpBeforeRerun.m Line 369-375: SPMdir and SPMpath are in a subfield.

Removed them from the list. They are in x.D now ae1eb76. It didn't crash because if this though, right?

Edit: Do all of the other ones really still exist? Like PathPop_MaskSusceptibility etc.?

jan-petr

Functions/xASL_Iteration.m:46: if ~isfield(x,'stopAfterErrors')
DataParTemplate.json:5: "exclusion": "",
Functions/xASL_init_DefineStudyData.m:93:if ~isfield(x,'exclusion')
Functions/xASL_init_DefineStudyData.m:148:if ~isfield(x,'exclusion')

jan-petr · 2021-05-31T16:07:16Z

Damn, I thought I found all the isfield ones. They are a bit tricky with the search function. I can fix that

I have a better search :)

MichaelStritt · 2021-05-31T16:11:21Z

Functions/xASL_init_DefineDataDependentSettings.m:39:if ~isfield(x,'Pediatric_Template')

Hope it's better with 3300654 now 👍

HenkMutsaerts · 2021-05-31T16:15:13Z

Jan should do a find & replace course ;)

jan-petr

Functions/xASL_adm_CleanUpBeforeRerun.m:370: 'SPMDIR', 'SPMpath', 'STRUCT_TEMPLATE_IM', 'LOCKDIR', 'SUBJECTDIR',...

Functions/xASL_adm_CleanUpBeforeRerun.m

Move to x.dataset.TimePointSubjects

Move to x.dataset ...

Move to x.opts ...

Move to x.settings

Move to x.dir

Move to x.dataset

MichaelStritt added the revamp Restructuring of ExploreASL label May 28, 2021

MichaelStritt requested a review from jan-petr May 28, 2021 00:10

MichaelStritt self-assigned this May 28, 2021

MichaelStritt linked an issue May 28, 2021 that may be closed by this pull request

Restructure x fields #480

Closed

MichaelStritt requested a review from BeatrizPadrela May 28, 2021 14:42

jan-petr reviewed May 31, 2021

View reviewed changes

HenkMutsaerts self-requested a review May 31, 2021 12:51

HenkMutsaerts requested changes May 31, 2021

View reviewed changes

MichaelStritt requested review from HenkMutsaerts and jan-petr May 31, 2021 13:24

jan-petr requested changes May 31, 2021

View reviewed changes

HenkMutsaerts approved these changes May 31, 2021

View reviewed changes

jan-petr requested changes May 31, 2021

View reviewed changes

Functions/xASL_adm_CleanUpBeforeRerun.m Show resolved Hide resolved

MichaelStritt and others added 26 commits June 1, 2021 20:56

#480 Restructure (x.TimePointSubjects)

00c29f1

Move to x.dataset.TimePointSubjects

#480 Restructure (x.exclusion and others)

60ef1cb

Move to x.dataset ...

#480 Restructure (x.TotalSubjects and others)

aaf1430

Move to x.dataset ...

#480 Restructure (x.dataParType)

65f119b

Move to x.opts ...

#480 Restructure (x.MUTEXID & x.RERUN)

566c9f7

Move to x.settings

#480 Restructure (x.LockDir)

e0831dc

Move to x.dir

#480 Restructure (x.SUBJECTDIR)

e5a507b

Move to x.dir

#480 Minor fix (x.SUBJECTDIR)

0fb63e2

Move to x.dir

#480 Minor fixes related to MUTEXID

f3e0f52

#480 Minor fixes related to MUTEXID

cae3301

#480 Minor fix (x.nSubjectsSessions)

06211ba

Move to x.dataset

#480 Minor fix (x.nSessions)

fc47bad

Move to x.dataset

#480 Redo: WBmask

c9859d7

#480 Redo: skull

fdac177

#480 Redo: tools -> external

d8f41dc

#480 Redo: WBmask -> x.S.masks

bb474ab

#480 Redo: skull -> x.S.masks

483e551

#480 CleanUp: SPMdir & SPMpath

c28abc0

#480 PediatricTemplate

3aefe07

#480 Mostly missing settings field

2685e5a

#480 Mostly missing dataset field

c19ca35

#480 SUBJECTDIR

a4deebb

#480 dryrun -> dryRun

f121f72

#480 xASL_io_ReadDataPar update deprecated parameters to new name

afd4a26

#480 previously missed renamed parameters

e7c3a4b

#480 xASL_adm_LoadX update deprecated parameters

e0bf14e

jan-petr force-pushed the revamp-#480_Restructure branch from dabf88a to e0bf14e Compare June 1, 2021 18:58

#480 xASL_adm_LoadX skill renaming of skull and WBMask

7023b8f

jan-petr merged commit 7023b8f into develop Jun 1, 2021

jan-petr deleted the revamp-#480_Restructure branch June 1, 2021 19:03

Revamp #480 restructure #607

Revamp #480 restructure #607

Uh oh!

Conversation

MichaelStritt commented May 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Linked issue

How to test

Comments

Uh oh!

jan-petr left a comment

Choose a reason for hiding this comment

Uh oh!

MichaelStritt commented May 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jan-petr commented May 31, 2021

Uh oh!

MichaelStritt commented May 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HenkMutsaerts left a comment

Choose a reason for hiding this comment

Uh oh!

MichaelStritt commented May 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HenkMutsaerts commented May 31, 2021

Uh oh!

jan-petr commented May 31, 2021

Uh oh!

MichaelStritt commented May 31, 2021

Uh oh!

jan-petr left a comment

Choose a reason for hiding this comment

Uh oh!

jan-petr left a comment

Choose a reason for hiding this comment

Uh oh!

jan-petr left a comment

Choose a reason for hiding this comment

Uh oh!

jan-petr commented May 31, 2021

Uh oh!

MichaelStritt commented May 31, 2021

Uh oh!

MichaelStritt commented May 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jan-petr left a comment

Choose a reason for hiding this comment

Uh oh!

jan-petr commented May 31, 2021

Uh oh!

MichaelStritt commented May 31, 2021

Uh oh!

HenkMutsaerts commented May 31, 2021

Uh oh!

jan-petr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MichaelStritt commented May 28, 2021 •

edited

Loading

MichaelStritt commented May 31, 2021 •

edited

Loading

MichaelStritt commented May 31, 2021 •

edited

Loading

MichaelStritt commented May 31, 2021 •

edited

Loading

MichaelStritt commented May 31, 2021 •

edited

Loading