Skip to content
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.

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

Update age sub-ucl06 #98

Merged
merged 2 commits into from
Oct 28, 2021
Merged

Update age sub-ucl06 #98

merged 2 commits into from
Oct 28, 2021

Conversation

alexfoias
Copy link
Contributor

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

Copy link
Member

@jcohenadad jcohenadad left a comment

Choose a reason for hiding this comment

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

how come it says that all lines are modified?

@joshuacwnewton
Copy link
Member

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 ?

@jcohenadad
Copy link
Member

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
Copy link
Contributor

kousu commented Oct 27, 2021

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
Copy link
Member

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
Copy link
Contributor Author

I tried from the GH web interface on PR #99 and only 2 lines were modified (I just changed the 6 to a 3 for sub-ucl06).

@kousu
Copy link
Contributor

kousu commented Oct 28, 2021

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
Copy link
Contributor

kousu commented Oct 28, 2021

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
Copy link
Contributor

kousu commented Oct 28, 2021

@jcohenadad there's your 30 second review afterall.

@alexfoias
Copy link
Contributor Author

@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
Copy link
Contributor

kousu commented Oct 28, 2021

You should start by

git config --global --unset core.crlf

And I'll update .gitattributes to include the better advice I figured out in neuropoly/data-management#117

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.

4 participants