Skip to content

Closes #1431 xASL_adm_GetPopulationSessions: regexp bugfix#1432

Merged
jan-petr merged 2 commits intodevelopfrom
bug-#1431_xASL_adm_GetPopulationSessions
Jul 4, 2023
Merged

Closes #1431 xASL_adm_GetPopulationSessions: regexp bugfix#1432
jan-petr merged 2 commits intodevelopfrom
bug-#1431_xASL_adm_GetPopulationSessions

Conversation

@HenkMutsaerts
Copy link
Member

@HenkMutsaerts HenkMutsaerts commented Jun 5, 2023

Linked issue

Closes #1431

@HenkMutsaerts HenkMutsaerts linked an issue Jun 5, 2023 that may be closed by this pull request
2 tasks
@HenkMutsaerts HenkMutsaerts requested a review from jan-petr June 5, 2023 18:41
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.

I do not understand why you mention PCASL_3D. The part of your code that you have modified isn't about folderHiererachy but about session tokens. Session tokens can be only ASL_1 or ASL_3 or ASL_13 etc. But never PCASL_3D, unless somebody writes nonsense to sourcestructure.json.

Also - I do not understand why you mention last folder layers in the issue. The changes you made apply to sessions and not folderHierarchy.

I agree with the code changes, just want to clarify the issue etc. Or have I overlooked something??

@HenkMutsaerts
Copy link
Member Author

I do not understand why you mention PCASL_3D. The part of your code that you have modified isn't about folderHiererachy but about session tokens. Session tokens can be only ASL_1 or ASL_3 or ASL_13 etc. But never PCASL_3D, unless somebody writes nonsense to sourcestructure.json.

Also - I do not understand why you mention last folder layers in the issue. The changes you made apply to sessions and not folderHierarchy.

I agree with the code changes, just want to clarify the issue etc. Or have I overlooked something??

Haha think about it. If you search for any ASL_3, there is an occurrence in PC ASL_3 D ;)

@jan-petr
Copy link
Contributor

I do not understand why you mention PCASL_3D. The part of your code that you have modified isn't about folderHiererachy but about session tokens. Session tokens can be only ASL_1 or ASL_3 or ASL_13 etc. But never PCASL_3D, unless somebody writes nonsense to sourcestructure.json.
Also - I do not understand why you mention last folder layers in the issue. The changes you made apply to sessions and not folderHierarchy.
I agree with the code changes, just want to clarify the issue etc. Or have I overlooked something??

Haha think about it. If you search for any ASL_3, there is an occurrence in PC ASL_3 D ;)

Yes. I am thinking about it :) It's a list of sessions, not folderHierarchy. The values can be only ASL_1 or ASL_2 etc. Where would PCASL_3D come from in that list? We are talking about a list of sessions here, not about folder names. If I get it right.

Also - check my other comments.

@HenkMutsaerts
Copy link
Member Author

HenkMutsaerts commented Jun 30, 2023

I do not understand why you mention PCASL_3D. The part of your code that you have modified isn't about folderHiererachy but about session tokens. Session tokens can be only ASL_1 or ASL_3 or ASL_13 etc. But never PCASL_3D, unless somebody writes nonsense to sourcestructure.json.
Also - I do not understand why you mention last folder layers in the issue. The changes you made apply to sessions and not folderHierarchy.
I agree with the code changes, just want to clarify the issue etc. Or have I overlooked something??

Haha think about it. If you search for any ASL_3, there is an occurrence in PC ASL_3 D ;)

Yes. I am thinking about it :) It's a list of sessions, not folderHierarchy. The values can be only ASL_1 or ASL_2 etc. Where would PCASL_3D come from in that list? We are talking about a list of sessions here, not about folder names. If I get it right.

Also - check my other comments.

What the code did: was checking for ANY occurrence of ASL_\d+ and taking the first instance, which would detect /Siemens_PCASL_3D_Test/sourcedata/sub-001/ASL_1 as ASL_3 (from PCASL_3D).

I improved this now in two ways:

  1. ensuring it searches for ASL_d+.nii
  2. taking the last index only

You're right that (2) is not really necessary anymore, I just kept it there to be sure.

@jan-petr jan-petr self-requested a review July 4, 2023 12:21
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.

@jan-petr jan-petr force-pushed the bug-#1431_xASL_adm_GetPopulationSessions branch from 89b5b5d to aee7dfd Compare July 4, 2023 12:23
@jan-petr jan-petr merged commit aee7dfd into develop Jul 4, 2023
@jan-petr jan-petr deleted the bug-#1431_xASL_adm_GetPopulationSessions branch July 4, 2023 12:23
@jan-petr jan-petr self-assigned this Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

xASL_adm_GetPopulationSessions regexp wrong

2 participants