-
Notifications
You must be signed in to change notification settings - Fork 8
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
Option for gradient coloring and alpha in edges #36
Conversation
Okay, I see some mypy errors, working on it. |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Hey, thank you for opening the MR. You can install pre-commit locally with |
I managed to resolve these errors. I also made a mistake during the process, so I have several commits. If that's a problem, please use the squash merge. Additionally, a thorough review could be necessary, because I don't exactly understand some part of the code. |
Hey ! So, I upgraded everything in #37 so the tests are now passing on main, if you rebase on it all the issues in the CI will come from the new changes. |
It's done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to support python 3.8 so we need a from __future__ import annotations
at the top of the file (thank you for adding typing!)
I used the new (from 3.10) type union operator (number: int | float,), but I will replace it with the older one (number: Union[int, float]). |
There was a complexity problem in the extended plot_strips method.
This was because it is checked that whether the input is a tuple. As the alpha was computed the same way regardless it is a gradient strip or not, I relocated that part before the gradient check. My test cases still work as expected. |
You can use the new syntax, but you need to add |
Okay, but I wanted consistent code style and Union was used at other parts. In another run, I replaced the PEP 585 style tuple[str] annotations to the pre 3.9 variant Tuple[str] (from typing namespace). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thank you for making the test pass !
Sorry for the multiple round that was required to make them pass. Thanks for your time. If I may, I think the version 1.5.0 would be better if you use semantic versioning as this is a new feature. |
Ha yes, my bad and it's already released now. I added you as collaborator you should be able to add a new tag and release from the github interface (push a tag then create a release from github interface) don't hesitate to release in the future :) |
As I'm really interested in the gradient option I adopted @fransua's code from #11.
In general I have some concerns regarding the code structure, but for now all I did was that I copied the relevant parts from the closed PR and make it work with the current state of the code.
I also added a notebook, called gradient_test where you can see the results. But for some reason I had to comment the
from pysankey.sankey.exceptions import LabelMismatch, NullsInFrame, PySankeyException
to be able to import the code from my notebook. It was restored before I opened this PR.With the new parts it is now possible to do the following:
no alpha, gradient
no alpha, no gradient
alpha and gradient
alpha, but no gradient
@Pierre-Sassoulas I hope it can merged as soon as possible, as I would like to see it in a release. Tell me it it still needs some care.
Thanks,