-
Notifications
You must be signed in to change notification settings - Fork 65
Tsdiffana quality checks #100
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
Conversation
|
I rebased but I haven't checked that the tsdiffana reports work fine yet. |
OK, nice. A quick way to check would be to run: OK, I've just took a deep look at the code and output. There're are couple of minor issues.
I'll roll a quick fix for all but the last issue and PR the changes to you. |
@lesteve: refer to PR I sent you. OK, this is the way things look now. But
Hope this helps. |
|
1632058
to
6f79035
Compare
Once PR #108 is merged, you should be fine. |
Any update ? |
I rebased on master but I still have a check_niimg that we added and needs fixing. I probably won't have time to look at this before next week. |
afcafbf
to
30e3ea1
Compare
# plot of mean volume variance | ||
ax = next(iter_axes) | ||
ax.plot(results['volume_mean_diff2'] / mean_means) | ||
xmax_labels(ax, T - 1, 'Difference image number', 'Scaled variance') |
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.
@bthirion all the xmax_labels calls are where you can improve of the axes titles.
OK, looks good. Only thing left now is the tooltips explaining the thumbnails the user is looking at. We don't need these to be over-elaborate. Once this is done, it's a merge. |
|
I updated the todo list at the top of this issue. I think there are a few more things remaining though. |
I've sent you a PR for some minor aspects. |
algorithms folder is not used anymore
in nilearn
Conflicts: pypreprocess/time_diff.py
Conflicts: Makefile
which is not that crucial Plus some PEP8
Nice! We still have the tooltip issue though (we need a different / On Mon, Jun 1, 2015 at 10:43 AM, Loïc Estève notifications@github.com
DED |
But that can be done separately from this PR I reckon. |
No, it can't be done separately. The PR is not complete without this. Otherwise the thumbs would be meaningless. |
On 01/06/2015 11:27, DOHMATOB Elvis wrote:
B |
and minor naming changes
("Average signal over each volume. A large drop/peak (e.g. 5%) indicates " | ||
"an artefact."), | ||
("Variance index per slice. Note that aqquisition artifacts can be slice" | ||
"-specific. Look at the day if there is a peak somewhere."), |
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.
@bthirion "Look at the day" 😕. Should it be "Look at the data instead"?
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 ;)
OK, things look much better now. I'll merge and then we'll improve things as we go along. |
supersedes #97.
Creating the PR to get the ball rolling.