adding charting function for Sankey diagram #95
Conversation
There was a problem hiding this comment.
Looks interesting! Left a few suggestions and notes that might help pass CI. It might also be reasonable to simply add these new dependencies to the dev-dependencies so that users can add them if they decide to use this feature (currently pandas is setup like that).
dune_client/viz/graphs.py
Outdated
| predefined_colors: dict, | ||
| columns: dict, | ||
| viz_config: dict, |
There was a problem hiding this comment.
the test suggests that these could be
| predefined_colors: dict, | |
| columns: dict, | |
| viz_config: dict, | |
| predefined_colors: dict[str, str], | |
| columns: dict[str, str], | |
| viz_config: dict[str, int | float], |
Although it also kinda looks like columns could be list[str].
Note that earlier python versions might require Union[int, float] (if you decide to go with this suggestion).
tests/unit/test_viz_sankey.py
Outdated
| "UNI": "rgb(255, 21, 126)", | ||
| } | ||
|
|
||
| self.columns = {"source": "source", "target": "target", "value": "value"} |
There was a problem hiding this comment.
Why are the columns mappings of themselves to themselves? Is there an example where they aren't just the same values mapping to themselves or does this even need to be a dict?
There was a problem hiding this comment.
I was thinking of giving folks the option to name their columns differently, which ultimately has to be mapped to be source, target, value.
There was a problem hiding this comment.
Perhaps this fact can be included somewhere in the doc strings. And also here in the test you could use targets like "renamed_column" to make things extra clear.
The test should serve as simple example that demonstrates all the way the function can be used.
There was a problem hiding this comment.
got it makes sense, changed!
agaperste
requested review from
bh2smith
and removed request for
msf and
andrewhong5297
|
@bh2smith Thanks for giving me all the pointers! When you get a chance, can you help me with this error from black? I ran stuff in local and the formatting is okay, but on GitHub it's still failing |
There was a problem hiding this comment.
I ran make check and black formatter just found and fixed the issues. Have you tried using the Makefile?
I found some type issues as well and took care of them. These two changes are each in their own commit if you want to see what the relevant changes were for each. You should be good to go now.
Unfortunately its not going to be an easy task to add type annotations to these ignored projects so we will have to live with ignore statements for now.
Everything looks alright to me although I have not explicitly tried the functionality. You have my approval, but it might not hurt to get someone a Dune approval as well.
Thanks for your contribution!
Co-authored-by: Benjamin Smith <bh2smith@users.noreply.github.com>
|
@bh2smith Yes I ran |
|
@msf When you get a chance, could you take a look at this PR? First time merging into the repo so wanted to be sure :) Thanks in advance! 🙏 |
|
I'm excited to see this in action. I might even use it! |
Starting to add functions for more charting capabilities, starting with making a Sankey diagram.