-
Notifications
You must be signed in to change notification settings - Fork 15
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
add site data ucdavis #77
Conversation
We didn't get the date_of_scan info. I think we can go without it. |
participants.tsv
Outdated
@@ -227,6 +227,13 @@ sub-ubc03 F 24 2019-06-06 ubc "University of British Columbia (UBC), Vancouver, | |||
sub-ubc04 F 25 2019-06-06 ubc "University of British Columbia (UBC), Vancouver, Canada" Philips Ingenia-ElitionX 16chHeadNeck+Posterior R5.5 "A.V. Dvorak, E. MacMillan, S. Kolind" | |||
sub-ubc05 M 24 2019-06-07 ubc "University of British Columbia (UBC), Vancouver, Canada" Philips Ingenia-ElitionX 16chHeadNeck+Posterior R5.5 "A.V. Dvorak, E. MacMillan, S. Kolind" | |||
sub-ubc06 M 23 2019-06-17 ubc "University of British Columbia (UBC), Vancouver, Canada" Philips Ingenia-ElitionX 16chHeadNeck+Posterior R5.5 "A.V. Dvorak, E. MacMillan, S. Kolind" | |||
sub-ucdavis01 F 32 - ucdavis "UC Davis Health, California, US" Siemens Magentom Vida HeadNeck_64_CS syngo MR XA20 Allan R. Martin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: Magentom --> Magnetom
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, no need to mention "Magnetom". We don't do it for Prisma, Skyra, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 8143f98
participants.tsv
Outdated
@@ -227,6 +227,13 @@ sub-ubc03 F 24 2019-06-06 ubc "University of British Columbia (UBC), Vancouver, | |||
sub-ubc04 F 25 2019-06-06 ubc "University of British Columbia (UBC), Vancouver, Canada" Philips Ingenia-ElitionX 16chHeadNeck+Posterior R5.5 "A.V. Dvorak, E. MacMillan, S. Kolind" | |||
sub-ubc05 M 24 2019-06-07 ubc "University of British Columbia (UBC), Vancouver, Canada" Philips Ingenia-ElitionX 16chHeadNeck+Posterior R5.5 "A.V. Dvorak, E. MacMillan, S. Kolind" | |||
sub-ubc06 M 23 2019-06-17 ubc "University of British Columbia (UBC), Vancouver, Canada" Philips Ingenia-ElitionX 16chHeadNeck+Posterior R5.5 "A.V. Dvorak, E. MacMillan, S. Kolind" | |||
sub-ucdavis01 F 32 - ucdavis "UC Davis Health, California, US" Siemens Magentom Vida HeadNeck_64_CS syngo MR XA20 Allan R. Martin | |||
sub-ucdavis02 F 56 - ucdavis "UC Davis Health, California, US" Siemens Magentom Vida HeadNeck_64_CS syngo MR XA21 Allan R. Martin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace HeadNeck_64_CS --> HeadNeck_64 (let's try to standardize a bit...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 8143f98
participants.tsv
Outdated
@@ -227,6 +227,13 @@ sub-ubc03 F 24 2019-06-06 ubc "University of British Columbia (UBC), Vancouver, | |||
sub-ubc04 F 25 2019-06-06 ubc "University of British Columbia (UBC), Vancouver, Canada" Philips Ingenia-ElitionX 16chHeadNeck+Posterior R5.5 "A.V. Dvorak, E. MacMillan, S. Kolind" | |||
sub-ubc05 M 24 2019-06-07 ubc "University of British Columbia (UBC), Vancouver, Canada" Philips Ingenia-ElitionX 16chHeadNeck+Posterior R5.5 "A.V. Dvorak, E. MacMillan, S. Kolind" | |||
sub-ubc06 M 23 2019-06-17 ubc "University of British Columbia (UBC), Vancouver, Canada" Philips Ingenia-ElitionX 16chHeadNeck+Posterior R5.5 "A.V. Dvorak, E. MacMillan, S. Kolind" | |||
sub-ucdavis01 F 32 - ucdavis "UC Davis Health, California, US" Siemens Magentom Vida HeadNeck_64_CS syngo MR XA20 Allan R. Martin | |||
sub-ucdavis02 F 56 - ucdavis "UC Davis Health, California, US" Siemens Magentom Vida HeadNeck_64_CS syngo MR XA21 Allan R. Martin | |||
sub-ucdavis03 M 41 - ucdavis "UC Davis Health, California, US" Siemens Magentom Vida HeadNeck_64_CS syngo MR XA22 Allan R. Martin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"syngo MR XA22" -> the last number is different across subjects... this is suspicious...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 8143f98
@jcohenadad I have updated the participants.tsv |
oopsi, any idea what's going on? |
@kousu The build fails. The error looks something like
Did I do something wrong? |
Probably that file didn't get upload to Amazon correctly. I'm investigating. |
Possibly because I'll see if I can figure out how to tweak the CI script to be able to deal with forks. But because of this separate branch problem |
It's a simpler, but related, problem: @alexfoias your local
(notice the timestamps) You need to do
More directly you can do
which is what I am thinking more and more I want to recommend. |
@kousu I did this:
|
@alexfoias Problem 2 seems to be that the files have not been uploaded to Amazon
Can you try this?
|
@kousu Still doesn't work https://gist.github.com/alexfoias/bdf8f28c3e2adbe8048cc3ce4aaf6736 |
@alexfoias oops, you need to load your Amazon password:
You need to run
You can put those in your ~/.profile (or, ~/.zprofile? You're on the new macOS right? is that the right path now?). I wonder if we can script this to make this easier. Personally, I have my AWS keys in my password manager so what I actually do is
which prompts me to unlock my password manager. I probably should have told you (the docs are here) |
Turns out @alexfoias had never had AWS access in the first place. I've granted him write-access to it. |
@kousu How do I have to add the AWS credentials ? Do I just have to set the env var |
@alexfoias yes, exactly. Just paste them into your terminal and press enter! You can also make them more permanent with these suggestions: #77 (comment) |
After exporting the AWS credentials:
|
@kousu Houston we have a problem :( |
@alexfoias |
Yeah looks like that's the problem:
It should say
|
|
Oh no, I'm not sure about that. |
It looks good on my local copy:
so I don't yet know what's wrong with Github. But my money is on Github not downloading the |
Yes, I think that's it: https://github.com/spine-generic/data-multi-subject/pull/77/checks?check_run_id=2363833281#step:8:123
so it's fetching the |
I'm going to mess with your branch a bit to figure this out. |
This is a classic case of neuropoly/data-management#67 |
I got it working. I'm going to merge now. |
sub-ucdavis04, sub-ucdavis05, sub-ucdavis06, sub-ucdavis07
😥 |
thank you @alexfoias and @kousu. These painful experiences add to cons of git-annex |
It's not fully git-annex's fault. It's just not fully compatible with the Github workflow. Equally much you could say "add to the cons of Github". |
https://github.com/spine-generic/data-multi-subject/actions/runs/756637528 😓 I predicted this just now:
okay I'll see what I can do about it. Right, of course, it's literally the same bug:
Clicking the 'merge' button doesn't also merge the What to do about that? Never use forks ever? Or never merge git-annex changes on the command line; but we have branch protection turned on so we cannot merge from the command line, it has to be done through the Github GUI? Or make sure to make associated PRs for the |
I'm trying my third strategy this time: #84. Can one of you approve it @jcohenadad @alexfoias ? |
yes, that's also a possibility. We could change our internal workflow, get the data somewhere, review them, and then someone uploads them directly on the master branch. |
@kousu Thanks for fixing the issue. Is there an update in the documentation for how we should handle data in the future ? |
Should we consider dropping forks/branches, and just having someone from the team review the addition and push it directly on master? |
We had a report by email ("git-annex at UMN servers") about these 51 files not being downloadable. I don't see how, CI has been green for the 8 months since they went in, but git annex is an arcane art and maybe the unusual fixup I did broke something somehow when combined with certain git annex versions. |
Curently missing sex, age, date of scan.
Here is a QC of defacing ucdavis_spineGeneric_QC.zip