10000 Tsdiffana quality checks by lesteve · Pull Request #100 · neurospin/pypreprocess · GitHub
[go: up one dir, main page]

Skip to content

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

Merged
merged 35 commits into from
Jun 1, 2015
Merged

Tsdiffana quality checks #100

merged 35 commits into from
Jun 1, 2015

Conversation

lesteve
Copy link
Contributor
@lesteve lesteve commented Apr 15, 2015

supersedes #97.

Creating the PR to get the ball rolling.

  • there is at least one TODO pertaining to the tsdiffana parameter documentation in a couple of functions. Suggestions welcome!
  • by removing plot_cv_tc completely I may have disregarded the session separating feature added in plot_cv_tc since
  • improve axis labels
  • there were some complaints about the colorbar during the Parietal meeting. I don't remember exactly what they were about unfortunately. Does anyone ?
  • tooltip for tsdiffana plots

@dohmatob
Copy link
Contributor
dohmatob commented May 4, 2015

@lesteve
Copy link
Contributor Author
lesteve commented May 4, 2015

I rebased but I haven't checked that the tsdiffana reports work fine yet.

@dohmatob
Copy link
Contributor
dohmatob commented May 5, 2015

OK, nice.

A quick way to check would be to run: ipython examples/nipype_preproc_spm_auditory.py --pdb.
When it's done, point your browser (firefox) to the indicated output html file, then click on the preproc tab.

OK, I've just took a deep look at the code and output. There're are couple of minor issues.

  • failures when rotations present in affine header (solve using reorder_img)
  • manual tweaking of dimensions of figures. We avoid this since the tweaking is done front-end in html
  • use_same_figure param is not very useful
  • It be nice to fill-in the tsdiffana_tooltip (it's marked TODO)

I'll roll a quick fix for all but the last issue and PR the changes to you.

@dohmatob
Copy link
Contributor
dohmatob commented May 6, 2015

@lesteve: refer to PR I sent you.

OK, this is the way things look now.

screenshot from 2015-05-06 07 38 45

But

  • There is still the missing tooltip problem/
  • The missing legends and labels

Hope this helps.

@dohmatob
Copy link
Contributor
dohmatob commented May 6, 2015
  • Also, it'd be nice to have better titles / legends than "slice_diff2_max_vol" or "diff2_mean_vol".

@dohmatob
Copy link
Contributor

Once PR #108 is merged, you should be fine.

@dohmatob
Copy link
Contributor

Any update ?

@lesteve
Copy link
Contributor Author
lesteve commented May 21, 2015

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.

@lesteve lesteve force-pushed the tsdiffana branch 2 times, most recently from afcafbf to 30e3ea1 Compare May 27, 2015 07:21
# 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')
Copy link
Contributor Author

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.

@dohmatob
Copy link
Contributor

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.

@dohmatob
Copy link
Contributor

@lesteve
Copy link
Contributor Author
lesteve commented May 27, 2015

Only thing left now is the tooltips explaining the thumbnails the user is looking at

I updated the todo list at the top of this issue. I think there are a few more things remaining though.

@bthirion
Copy link
Member

I've sent you a PR for some minor aspects.

@lesteve
Copy link
Contributor Author
lesteve commented Jun 1, 2015

I just added the session separation, any comments let me know!

pypreprocess_session_separation

Looks like I will need to rebase again but other than that I reckon all the todo list items have been tackled.

BTW the snapshot is from running examples/nipype_preproc_spm_multimodal_faces.py.

@dohmatob
Copy link
Contributor
dohmatob commented Jun 1, 2015

Nice! We still have the tooltip issue though (we need a different /
specific tooltip string per thumbnail).

On Mon, Jun 1, 2015 at 10:43 AM, Loïc Estève notifications@github.com
wrote:

I just added the session separation, any comments let me know!

[image: pypreprocess_session_separation]
https://cloud.githubusercontent.com/assets/1680079/7909207/e04b1e72-084a-11e5-882c-29a68f06e688.png

Looks like I will need to rebase again but other than that I reckon all
the todo list items have been tackled.


Reply to this email directly or view it on GitHub
#100 (comment)
.

DED

@lesteve
Copy link
Contributor Author
lesteve commented Jun 1, 2015

Nice! We still have the tooltip issue though (we need a different / specific tooltip string per thumbnail).

But that can be done separately from this PR I reckon.

@dohmatob
Copy link
Contributor
dohmatob commented Jun 1, 2015

No, it can't be done separately. The PR is not complete without this. Otherwise the thumbs would be meaningless.

@bthirion
Copy link
Member
bthirion commented Jun 1, 2015

On 01/06/2015 11:27, DOHMATOB Elvis wrote:

No, it can't be done in a separate PR. The PR is not complete without
this. Otherwise the thumbs would be meaningless.


Reply to this email directly or view it on GitHub
#100 (comment).

But I've sent a PR to Loic, haven't I ?

B

@dohmatob
Copy link
Contributor
dohmatob commented Jun 1, 2015

OK, cool. So I guess we'll be fine once @bthirion's PR (to @lesteve) is merged into this PR.

("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."),
Copy link
Contributor Author

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"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed ;)

@dohmatob
Copy link
Contributor
dohmatob commented Jun 1, 2015

OK, things look much better now. I'll merge and then we'll improve things as we go along.

dohmatob added a commit that referenced this pull request Jun 1, 2015
@dohmatob dohmatob merged commit 072e555 into neurospin:master Jun 1, 2015
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.

3 participants
0