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
Addressing issue #525: Added helper for stacked #870
Addressing issue #525: Added helper for stacked #870
Conversation
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.
@naresh-bachwani this looks great and very complete! I have a few minor stylistic comments, but I think the next step will be to add tests!
Tests go in tests/test_draw.py
and we should probably test the following cases:
- vertically stacked barchart
- horizontal stacked barchart
- barchart with only one row
- barchart with labels and ticks supplied
That seems like it should be sufficient to get us good coverage, what do you think?
Also would you mind looking to see if there are docs for this package? (I suspect there are not, but I just want to make sure).
Thank you so much for your hard work! All of this looks really good!
The data associated with the bar chart where the columns represent each bar | ||
and the rows represent each stack in the bar chart. A single bar chart would | ||
be a 2D array with only one row, a bar chart with three stacks per bar would | ||
have a shape of (3, b). |
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 might need to make this a bit more readable and understandable. @DistrictDataLabs/team-oz-maintainers does this make sense to you? Any thoughts?
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.
I picked it up from the #525. It had a basic function signature. We can change this if you suggest.
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.
Looks great! Very good tests and baseline images. Right now the tests are failing with very low tolerances e.g. some are failing with 0.025 and others with 0.071 -- I think all we need to do is update the tolerances as I suggest below and the tests will pass and we can merge this in! May I ask what operating system you're using to generate the baseline images? We've noticed that baseline images on Windows generate slightly different images particularly if there are fonts involved.
tests/test_draw.py
Outdated
assert ticks_ax==ticks | ||
|
||
# Assert image similarity | ||
self.assert_images_similar(ax=ax) |
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.
Please add tol=0.05
to this test.
tests/test_draw.py
Outdated
assert ticks_ax==ticks | ||
|
||
# Assert image similarity | ||
self.assert_images_similar(ax=ax) |
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.
Please add tol=0.05
to this test.
@naresh-bachwani looks good - I'll update with the latest from develop then merge this in! Would you mind opening a new PR for updating the visualizers that have stacked bar charts to use this helper? |
I am using windows10 to generate baseline images. And hence the difference. I experienced same issue in one of my previous PR and Rebecca and Larry helped me out there! |
Awesome! Then let's |
This PR adds a helper function to help with stack bar-chart functionality.
I have made the following changes
Code to obtain below plots:
The sample plot for the above changes are as follows:
Still to do: