FIX: correct label orientation for 0-90 deg nodes in plot_connectivity_circle#13855
Conversation
|
Hello! 👋 Thanks for opening your first pull request here! ❤️ We will try to get back to you soon. 🚴 |
|
It looks like this branch includes several unrelated commits or working-tree changes. Could you reset/rebase the branch onto current main and recommit just the relevant files? Based on the commit history, three commits are unrelated to the current work |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
paavalipopov
force-pushed
the
fix-label-orientation-circle
branch
from
7e49ef8 to
f96ce9f
Compare
|
Hey @paavalipopov Can you please create a CircleCI account by signing into CircleCI via GitHub? After that you can push a commit here, and the documentation build job should run. |
CarinaFo
left a comment
CarinaFo
left a comment
There was a problem hiding this comment.
Thank you and congrats to your first PR, looks good to me.
|
Thanks for the review and the help with the cleanup, @CarinaFo! Glad to contribute to mne tools. |
drammock
left a comment
drammock
left a comment
There was a problem hiding this comment.
thanks @paavalipopov this is a nice PR. Thanks for the thorough test. I have a couple of suggestions to tidy it a bit, which I think should be uncontroversial so I'll apply them and mark for merge.
| rng = np.random.default_rng(0) | ||
| con = rng.uniform(0, 1, size=(n_nodes, n_nodes)) |
There was a problem hiding this comment.
no need to bother with randomness in a test that only cares about label orientation
| rng = np.random.default_rng(0) | |
| con = rng.uniform(0, 1, size=(n_nodes, n_nodes)) | |
| con = np.ones((n_nodes, n_nodes)) |
There was a problem hiding this comment.
could also do 1 - np.eye(n_nodes) and avoid the fill diagonal but that seems a bit too clever, might puzzle a future maintainer
There was a problem hiding this comment.
this breaks the _plot_connectivity_circle function (division by zero error in the line where the connectivity values get normalised). From what I understand, this is used to map the colors according to connectivity strength and should probably be addressed in a new PR as it is a edge case within plot_connectivity_circle.
There was a problem hiding this comment.
Oops. PR is not merged yet so revert this to using random values I guess
There was a problem hiding this comment.
I modified the values in the connectivity matrix to add some variability without using rng, it works now.
con = np.ones((n_nodes, n_nodes))
con[::2, ::2] = 0 # add some zeros to avoid 0-div in normalization
Co-authored-by: Daniel McCloy <dan@mccloy.info>
Head branch was pushed to by a user without write access
|
It seems like build_docs check is failing due to a warning in the mne.viz.ui_events.disable_ui_events docstring. I did not touch this function, is there anything I should do on my end to fix it? |
|
CircleCI failure is being fixed in #13868, will merge manually once other CIs come back green |
|
🎉 Congrats on merging your first pull request! 🥳 Looking forward to seeing more from you in the future! 💪 |
4 participants
Reference issue (if any)
What does this implement/fix?
In
mne.viz.circle._plot_connectivity_circle, the condition on line 341if angle_deg >= 270that determines labels horizontal alignment only covers the 270–360° range (3–6 o'clock). Nodes in the 0–90° range (12–3 o'clock) are incorrectly treated as left-half nodes, and their labels appear pointing inward, which looks like a bug.Fix: extend the condition to
angle_deg >= 270orangle_deg < 90, matching the full right half of the circle.Additional information
Here's a visual comparison of connectograms generated using the original ("old") and patched ("new") versions of mne-tools.