[chord_diagram.chord_diagram] optarg 'order' now also applies to the arch colors #32

Merged
tfardet merged 1 commits from gph82/mpl_chord_diagram:fix_colors into main 4 months ago
gph82 commented 4 months ago
Collaborator

Here we go again ;)

This one isn't urgent.

It feels like 'order' should also reorder fragment colors, specially if it reorders fragment names, right?

names=["A","B","C"]
colors=["red","green","blue"]

When passing the above arguments, the user probably wants "A"->"red", "B"->"green", "C"->"blue" regardless of order being [0,1,2], [2,1,0] or whatever.

E.g., when trying different orders, you want a each arch conserving color accross trials, to be able to "follow" them visually, right?

PS: I accidentally (pushed directly from terminal) created the branch 'fix_colors' in your original repo instead of my fork. I deleted it already, I think I should never push to yours. That was an accident, sorry!

EDIT: I closed the previous PR because I re-worded the commit message (fragments->chords)

Here we go again ;) This one isn't urgent. It feels like 'order' should also reorder fragment colors, specially if it reorders fragment names, right? ``` names=["A","B","C"] colors=["red","green","blue"] ``` When passing the above arguments, the user probably wants "A"->"red", "B"->"green", "C"->"blue" regardless of order being [0,1,2], [2,1,0] or whatever. E.g., when trying different orders, you want a each arch conserving color accross trials, to be able to "follow" them visually, right? PS: I accidentally (pushed directly from terminal) created the branch 'fix_colors' in your original repo instead of my fork. I deleted it already, I think I should never push to yours. That was an accident, sorry! EDIT: I closed the previous PR because I re-worded the commit message (fragments->chords)
gph82 added 1 commit 4 months ago
tfardet approved these changes 4 months ago
tfardet left a comment

OK, no need to make a new PR just to change commit messages, I'm anyway squashing commits and potentially rewording.

Poster
Collaborator

ah, I see. Well then of course, we do it your preferred way. Should you merge or do I do it? Anyways Thanks a lot!

ah, I see. Well then of course, we do it your preferred way. Should you merge or do I do it? Anyways Thanks a lot!
tfardet merged commit 97cdcaa26d into main 4 months ago
tfardet added the
enhancement
behavior-change
labels 4 months ago
gph82 deleted branch fix_colors 4 months ago
Owner

I forgot I gave you merge rights.
I think in the future it's better if the person who did not author the PR merges it.

I forgot I gave you merge rights. I think in the future it's better if the person who did not author the PR merges it.

Reviewers

tfardet approved these changes 4 months ago
The pull request has been merged as 97cdcaa26d.
Sign in to join this conversation.
Loading…
There is no content yet.