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

[WIP] add plot_anat_landmarks function #824

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

agramfort
Copy link
Member

let me know what you think

add plot_anat_landmarks function
@codecov-commenter
Copy link

codecov-commenter commented Jul 4, 2021

Codecov Report

Merging #824 (0c8f5d0) into main (a552a53) will decrease coverage by 0.53%.
The diff coverage is 18.51%.

@@            Coverage Diff             @@
##             main     #824      +/-   ##
==========================================
- Coverage   95.20%   94.67%   -0.54%     
==========================================
  Files          24       25       +1     
  Lines        3860     3887      +27     
==========================================
+ Hits         3675     3680       +5     
- Misses        185      207      +22     
Impacted Files Coverage Δ
mne_bids/viz.py 18.51% <18.51%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@agramfort
Copy link
Member Author

ok this one should be good for review. See rendered doc:

https://4812-89170358-gh.circle-artifacts.com/0/dev/auto_examples/convert_mri_and_trans.html

I will add a what's new entry at the end.

print_dir_tree(output_path)

plot_anat_landmarks(t1w_bids_path, vmax=160)
plt.suptitle('T1 MRI')
Copy link
Member

Choose a reason for hiding this comment

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

Can this be moved into plot_anat_landmarks? It would be nice if the figure came with a sensible title by default

Copy link
Member

Choose a reason for hiding this comment

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

Would also be good to briefly explain what vmax is doing here?

examples/convert_mri_and_trans.py Outdated Show resolved Hide resolved
Comment on lines 152 to 154
# landmarks Nasion, LPA, and RPA onto the brain image. For that, we can
# extract the location of Nasion, LPA, and RPA from the MEG file, apply our
# transformation matrix :code:`trans`, and plot the results.
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if we should start off this paragraph differently or put it into a new subsection. The first sentence made me think, "umm, but we just DID that??" until I got to the point where it says, " ... from the MEG file"

mne_bids/tests/test_viz.py Outdated Show resolved Hide resolved
mne_bids/viz.py Outdated Show resolved Hide resolved
Comment on lines +18 to +19
vmax : float
Maximum colormap value.
Copy link
Member

Choose a reason for hiding this comment

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

Should we limit the acceptable range, e.g. to the interval [0, 255]?

mne_bids/viz.py Outdated Show resolved Hide resolved
mne_bids/viz.py Outdated
Comment on lines 25 to 27
fig : matplotlib.figure.Figure | None
The figure object containing the plot. None if no landmarks
are avaiable.
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we raise an exception if there are no landmarks?

mne_bids/viz.py Outdated
)

if show:
plt.show()
Copy link
Member

Choose a reason for hiding this comment

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

Should this better be fig.show()?

Copy link
Member

@sappelhoff sappelhoff left a comment

Choose a reason for hiding this comment

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

Why do we need this? Is nilearn's plot_anat not sufficient?

@sappelhoff sappelhoff added this to the 0.8 milestone Jul 8, 2021
@agramfort
Copy link
Member Author

@sappelhoff it's ok for me if we don't merge this. It was a very convenient tool for debugging recently and I see this as being potentially useful to check some existing dataset. but It's ok if we don't merge this. Maybe it's not useful in the end...

@sappelhoff
Copy link
Member

Okay, I'll leave the decision to you and @hoechenberger - I am 50-50

@agramfort
Copy link
Member Author

agramfort commented Jul 8, 2021 via email

@sappelhoff sappelhoff modified the milestones: 0.8, 0.9 Jul 9, 2021
@sappelhoff sappelhoff marked this pull request as draft July 9, 2021 12:55
@sappelhoff sappelhoff changed the title add plot_anat_landmarks function [WIP] add plot_anat_landmarks function Jul 9, 2021
@hoechenberger hoechenberger modified the milestones: 0.9, 0.10 Nov 10, 2021
@hoechenberger hoechenberger modified the milestones: 0.10, 0.11 Mar 2, 2022
@sappelhoff sappelhoff modified the milestones: 0.11, 0.12 Oct 8, 2022
@sappelhoff sappelhoff modified the milestones: 0.12, 0.13 Dec 18, 2022
@sappelhoff sappelhoff modified the milestones: 0.13, future (or never) Jul 27, 2023
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