tfardet
/
mpl_chord_diagram
2
0
Fork 1
Code Issues 4 Pull Requests 1 Projects Releases 4 Wiki Activity
Labels Milestones

[chord_diagram] optionally hide zero-width chords #14

Merged
gph82 merged 3 commits from master into master 1 year ago
Conversation 9 Commits 3 Files Changed 1
gph82 commented 2 years ago (Migrated from github.com)
Owner

Hi! You seem to have the most recent/active fork of mpl_chord_diagram.

It worked out of the box for my problem in 2 min, thanks!

Since you don't have "issues", I'm communicating through a PR, I hope that's okay.

So, while I decide whether to make another project of mine (gph82/mdciao) depend on mpl_chord_diagram (your fork or mine, also undecided), I noticed there's no option to turn off zero-width nodes, so I included one.

If you don't like cluttering the signature, I was thinking perhaps to make it depend on whether the matrix is given as sparse or dense. Sparse triggers the "don't show zeros" option internally automatically.

What do you think?

image

Hi! You seem to have the most recent/active fork of mpl_chord_diagram. It worked out of the box for my problem in 2 min, thanks! Since you don't have "issues", I'm communicating through a PR, I hope that's okay. So, while I decide whether to make another project of mine (gph82/mdciao) depend on mpl_chord_diagram (your fork or mine, also undecided), I noticed there's no option to turn off zero-width nodes, so I included one. If you don't like cluttering the signature, I was thinking perhaps to make it depend on whether the matrix is given as sparse or dense. Sparse triggers the "don't show zeros" option internally automatically. What do you think? ![image](https://user-images.githubusercontent.com/7518004/98798416-2727e480-240e-11eb-8ab4-dd0fb6895908.png)
Silmathoron commented 2 years ago (Migrated from github.com)
Owner

Hi there!
Sorry, I did not see your PR for some reason...
Thanks for contributing, I will try to look at it today!

Hi there! Sorry, I did not see your PR for some reason... Thanks for contributing, I will try to look at it today!
Silmathoron (Migrated from github.com) requested changes 2 years ago
Silmathoron (Migrated from github.com) left a comment
Owner

Ok, actually decided I should do that right away before I forget ^^
I think this definitely very relevant, thank you (should even become the default behavior).

If you can address the minor points that I raised, I'll merge it.
If you can, it would be nice if you could combine everything into one commit afterwards so I can rebase the PR (if I'm not mistaken, I think that should enable to avoid cluttering the log with useless merge commits while still preserving you authorship on the commit).

As for which fork to point to... I guess that's up to you... I can already say that my library (NNGT already points to this one so I'll try to maintain it actively.

PS : activated issues and (hopefully) turned on notifications to avoid having you wait 7 days next time again.

Ok, actually decided I should do that right away before I forget ^^ I think this definitely very relevant, thank you (should even become the default behavior). If you can address the minor points that I raised, I'll merge it. If you can, it would be nice if you could combine everything into one commit afterwards so I can rebase the PR (if I'm not mistaken, I think that should enable to avoid cluttering the log with useless merge commits while still preserving you authorship on the commit). As for which fork to point to... I guess that's up to you... I can already say that my library ([NNGT](https://nngt.readthedocs.io/) already points to this one so I'll try to maintain it actively. PS : activated issues and (hopefully) turned on notifications to avoid having you wait 7 days next time again.
chord_diagram.py
Silmathoron (Migrated from github.com) commented 2 years ago
Owner

Thinking about it, I think it is definitely reasonable to make show_zeros=False the default behavior (I know of no cases were people would expect to see non-existent connections)

Thinking about it, I think it is definitely reasonable to make ``show_zeros=False`` the default behavior (I know of no cases were people would expect to see non-existent connections)
chord_diagram.py
Silmathoron (Migrated from github.com) commented 2 years ago
Owner

change default (cf. above)

change default (cf. above)
chord_diagram.py
start, end = pos[(i, i)]
Silmathoron (Migrated from github.com) commented 2 years ago
Owner 
        if mat[i, i] > 0 or show_zeros:
```suggestion if mat[i, i] > 0 or show_zeros: ```
chord_diagram.py
start1, end1 = pos[(i, j)]
Silmathoron (Migrated from github.com) commented 2 years ago
Owner 
            if mat[i, j] > 0 or mat[j, i] > 0 or show_zeros:
```suggestion if mat[i, j] > 0 or mat[j, i] > 0 or show_zeros: ```
Silmathoron commented 2 years ago (Migrated from github.com)
Owner

OK, last thing I'm wondering about, since it might be useful to avoid clutter to the API, is whether we move show_zeros to **kwargs... any opinion?

OK, last thing I'm wondering about, since it might be useful to avoid clutter to the API, is whether we move ``show_zeros`` to ``**kwargs``... any opinion?
gph82 (Migrated from github.com) reviewed 2 years ago
chord_diagram.py
gph82 (Migrated from github.com) commented 2 years ago
Owner

Yeah, default should be zero. It's just I did not want to change API behavior in the PR.

Yeah, default should be zero. It's just I did not want to change API behavior in the PR.
Silmathoron (Migrated from github.com) reviewed 2 years ago
chord_diagram.py
Silmathoron (Migrated from github.com) commented 2 years ago
Owner

You have my blessing to do so, I'll also let you decide whether it should go into kwargs or not... I have no strong opinion on the matter

You have my blessing to do so, I'll also let you decide whether it should go into ``kwargs`` or not... I have no strong opinion on the matter
Silmathoron commented 1 year ago (Migrated from github.com)
Owner

Hi there, sorry just wanted to check whether you were actually planning on doing the full PR workflow so it's included with your authorship in the package or if this was just for discussion and I should open a new PR myself.

Hi there, sorry just wanted to check whether you were actually planning on doing the full PR workflow so it's included with your authorship in the package or if this was just for discussion and I should open a new PR myself.
gph82 commented 1 year ago (Migrated from github.com)
Owner

Hi, thanks, sorry, I forgot about this a bit. So I think show_zeros should be moved to kwargs or even deleted completely...is there any reason it should be kept? I was just being conservative when proposing changes

Hi, thanks, sorry, I forgot about this a bit. So I think show_zeros should be moved to kwargs or even deleted completely...is there any reason it should be kept? I was just being conservative when proposing changes
Silmathoron commented 1 year ago (Migrated from github.com)
Owner

Yeah, actually you're right, remove it completely 👍

Yeah, actually you're right, remove it completely :+1:
gph82 commented 1 year ago (Migrated from github.com)
Owner

What do you think?

What do you think?
Silmathoron (Migrated from github.com) requested changes 1 year ago
Silmathoron (Migrated from github.com) left a comment
Owner

Alright, just some tiny formatting left and we're good to go!

Alright, just some tiny formatting left and we're good to go!
chord_diagram.py
Silmathoron (Migrated from github.com) commented 1 year ago
Owner 
                  use_gradient=False, show=False, **kwargs):
```suggestion use_gradient=False, show=False, **kwargs): ```
chord_diagram.py
Silmathoron (Migrated from github.com) commented 1 year ago
Owner 
        if mat[i, i] > 0:
```suggestion if mat[i, i] > 0: ```
chord_diagram.py
Silmathoron (Migrated from github.com) commented 1 year ago
Owner 
            if mat[i, j] > 0 or mat[j, i] > 0:
```suggestion if mat[i, j] > 0 or mat[j, i] > 0: ```
Silmathoron (Migrated from github.com) approved these changes 1 year ago
Silmathoron (Migrated from github.com) left a comment
Owner

Actually I can just validate them myself ^^

Actually I can just validate them myself ^^
gph82 commented 1 year ago (Migrated from github.com)
Owner

LGTM, expect a new PR on other stuff in a few mins lol

LGTM, expect a new PR on other stuff in a few mins lol
Silmathoron commented 1 year ago (Migrated from github.com)
Owner

OK, great ^^
And GitHub apparently fixed their squash mechanism so I'm happy to say your authorship was correctly processed!

OK, great ^^ And GitHub apparently fixed their squash mechanism so I'm happy to say your authorship was correctly processed!
gph82 commented 1 year ago (Migrated from github.com)
Owner

wait a sec, some changes of the second PR got into this one while selectively committing, gimme a sec

wait a sec, some changes of the second PR got into this one while selectively committing, gimme a sec

Reviewers

Silmathoron
The pull request has been merged as 5b6e627e7f.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
Silmathoron
Labels
Apply labels
Clear labels
behavior-change
This changes the default behavior bug
Something isn't working bugfix documentation
Improvements or additions to documentation duplicate
This issue or pull request already exists enhancement
New feature or request good first issue
Good for newcomers help wanted
Extra attention is needed installation
About how the library is installed invalid
This doesn't seem right question
Further information is requested wontfix
This will not be worked on
No Label behavior-change bug bugfix documentation duplicate enhancement good first issue help wanted installation invalid question wontfix
Milestone
Set milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Assign users
Clear assignees
No Assignees
1 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'master'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes