spine-generic / data-multi-subject

Multi-subject data for the Spine Generic project
Creative Commons Attribution 4.0 International
22 stars 15 forks source link

Update age sub-ucl06 #98

Closed alexfoias closed 2 years ago

alexfoias commented 2 years ago

Update age based on email Re: Body measures - UCL, received on October 26, 2021 4:58 PM.

joshuacwnewton commented 2 years ago

The recommendation I made https://github.com/spinalcordtoolbox/spinalcordtoolbox/issues/3549may be the cause of this. :thinking:

See specifically: https://github.com/spinalcordtoolbox/spinalcordtoolbox/issues/3549#issuecomment-951236839.

I think it came from neuropoly/data-management@6fc0630 . @kousu Does it have any impact the autocrlf in our internal git-annex ? I think we had issues with partipicants.tsv ?

jcohenadad commented 2 years ago

The recommendation I made https://github.com/spinalcordtoolbox/spinalcordtoolbox/issues/3549may be the cause of this. 🤔

See specifically: spinalcordtoolbox/spinalcordtoolbox#3549 (comment).

I think it came from neuropoly/data-management@6fc0630 . @kousu Does it have any impact the autocrlf in our internal git-annex ? I think we had issues with partipicants.tsv ?

it could be it, thank you for chipping in @joshuacwnewton . So in this case what would be an "easy" way (ie: <30s) for me to review?

kousu commented 2 years ago

This change might be correct actually? If the dataset was using nonstandard line endings before and now it is?

If so, I can pull out the line ending changes to a PR separate from Alex's. And if not, I can reset this PR to only contain the actual change.

Hang on I'll dig into it. But I don't think 30s is a reasonable ask here, give me the space to do it right.

jcohenadad commented 2 years ago

But I don't think 30s is a reasonable ask here, give me the space to do it right.

i didn't mean "i'd like to review this in the next 30s", but rather "i'd like to be able to review it in a duration that is <30s"

alexfoias commented 2 years ago

I tried from the GH web interface on PR https://github.com/spine-generic/data-multi-subject/pull/99 and only 2 lines were modified (I just changed the 6 to a 3 for sub-ucl06).

kousu commented 2 years ago

Okay yes, I can see that the problem is the file was brokenin unix format and Alex inadvertently fixed itmade it into Windows format by removing adding the '\rs (which diff renders as ^Ms):

u108545@joplin:~/data-multi-subject$ git branch --show-current
af/update_sub-ucl06
u108545@joplin:~/data-multi-subject$ git log -p
commit a8ef61fcfc7729c6b4f5eb26a09422161b672158 (HEAD -> af/update_sub-ucl06, origin/af/update_sub-ucl06)
Author: alexfoias <afoias@polymtl.ca>
Date:   Wed Oct 27 11:48:30 2021 -0400

    update sub-ucl06

diff --git a/participants.tsv b/participants.tsv
index 7d1db11a..21313816 100644
--- a/participants.tsv
+++ b/participants.tsv
@@ -1,268 +1,268 @@
-participant_id sex     age     height  weight  date_of_scan    institution_id  institution     manufacturer    manufacturers_model_name        receive_coil_name       software_versions       researcher
-sub-amu01      M       28      -       -       2019-02-12      amu     AMU - CEMEREM   Siemens Verio   NeckMatrix      syngo_MR_B17    Virginie Callot
-sub-amu02      M       28      -       -       2019-02-13      amu     AMU - CEMEREM   Siemens Verio   NeckMatrix      syngo_MR_B17    Virginie Callot
-sub-amu03      F       28      -       -       2019-02-13      amu     AMU - CEMEREM   Siemens Verio   NeckMatrix      syngo_MR_B17    Virginie Callot
-sub-amu04      F       44      -       -       2019-03-01      amu     AMU - CEMEREM   Siemens Verio   NeckMatrix      syngo_MR_B17    Virginie Callot
-sub-amu05      F       39      -       -       2019-03-01      amu     AMU - CEMEREM   Siemens Verio   NeckMatrix      syngo_MR_B17    Virginie Callot
....
+participant_id sex     age     height  weight  date_of_scan    institution_id  institution     manufacturer    manufacturers_model_name        receive_coil_name       software_versions       researcher^M
+sub-amu01      M       28      -       -       2019-02-12      amu     AMU - CEMEREM   Siemens Verio   NeckMatrix      syngo_MR_B17    Virginie Callot^M
+sub-amu02      M       28      -       -       2019-02-13      amu     AMU - CEMEREM   Siemens Verio   NeckMatrix      syngo_MR_B17    Virginie Callot^M
+sub-amu03      F       28      -       -       2019-02-13      amu     AMU - CEMEREM   Siemens Verio   NeckMatrix      syngo_MR_B17    Virginie Callot^M
+sub-amu04      F       44      -       -       2019-03-01      amu     AMU - CEMEREM   Siemens Verio   NeckMatrix      syngo_MR_B17    Virginie Callot^M
+sub-amu05      F       39      -       -       2019-03-01      amu     AMU - CEMEREM   Siemens Verio   NeckMatrix      syngo_MR_B17    Virginie Callot^M
...

Which is doubly-confirmed by CI: https://github.com/spine-generic/data-multi-subject/runs/4024506569?check_suite_focus=true

Screenshot 2021-10-28 at 13-51-06 Build software better, together

I'll get the change out with unix2dosdos2unix.

kousu commented 2 years ago
u108545@joplin:~/data-multi-subject$ dos2unix participants.tsv 
dos2unix: converting file participants.tsv to Unix format...
u108545@joplin:~/data-multi-subject$ git commit -m "Repair lineendings"
[af/update_sub-ucl06 681495fe] Repair lineendings
 1 file changed, 267 insertions(+), 267 deletions(-)
u108545@joplin:~/data-multi-subject$ git diff master..
diff --git a/participants.tsv b/participants.tsv
index 7d1db11a..1c55b119 100644
--- a/participants.tsv
+++ b/participants.tsv
@@ -239,7 +239,7 @@ sub-ucl02   M       31      -       -       2018-11-21      ucl     University College London       Philips Achieva-dStr
 sub-ucl03      M       30      -       -       2018-11-21      ucl     University College London       Philips Achieva-dStream -       Release 5.4     "F. Grussu, M. Battiston, R. S. Samson, M. C. Yiannakas, C. A. M. Gandini Wheeler-Kingshott"
 sub-ucl04      F       37      -       -       2018-11-26      ucl     University College London       Philips Achieva-dStream -       Release 5.4     "F. Grussu, M. Battiston, R. S. Samson, M. C. Yiannakas, C. A. M. Gandini Wheeler-Kingshott"
 sub-ucl05      M       38      -       -       2018-11-28      ucl     University College London       Philips Achieva-dStream -       Release 5.4     "F. Grussu, M. Battiston, R. S. Samson, M. C. Yiannakas, C. A. M. Gandini Wheeler-Kingshott"
-sub-ucl06      F       36      -       -       2018-11-29      ucl     University College London       Philips Achieva-dStream -       Release 5.4     "F. Grussu, M. Battiston, R. S. Samson, M. C. Yiannakas, C. A. M. Gandini Wheeler-Kingshott"
+sub-ucl06      F       33      -       -       2018-11-29      ucl     University College London       Philips Achieva-dStream -       Release 5.4     "F. Grussu, M. Battiston, R. S. Samson, M. C. Yiannakas, C. A. M. Gandini Wheeler-Kingshott"
 sub-unf01      F       24      169     70      2018-12-07      unf     "Neuroimaging Functional Unit (UNF), CRIUGM, Polytechnique Montreal"    Siemens Prisma-fit      HeadNeck_64     syngo_MR_E11    "J. Cohen-Adad, A. Foias"
 sub-unf02      M       29      180     95      2018-12-07      unf     "Neuroimaging Functional Unit (UNF), CRIUGM, Polytechnique Montreal"    Siemens Prisma-fit      HeadNeck_64     syngo_MR_E11    "J. Cohen-Adad, A. Foias"
 sub-unf03      M       22      170     60      2018-12-07      unf     "Neuroimaging Functional Unit (UNF), CRIUGM, Polytechnique Montreal"    Siemens Prisma-fit      HeadNeck_64     syngo_MR_E11    "J. Cohen-Adad, A. Foias"
kousu commented 2 years ago

@jcohenadad there's your 30 second review afterall.

alexfoias commented 2 years ago

@kousu Thanks for looking into it. How should I change the files in the future to avoid this issue ? Or what config should I change ?

kousu commented 2 years ago

You should start by

git config --global --unset core.crlf

And I'll update .gitattributes to include the better advice I figured out in https://github.com/neuropoly/data-management/pull/117