Skip to content
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

Added month separator feature (#14) and fix the pandas warning (#10) #15

Closed
wants to merge 5 commits into from

Conversation

ma-schmidt
Copy link
Contributor

For the fix, the two issues that were raised in the previous PR were:

  1. The how argument to these functions is now ignored. Using the agg aggregation generic function, we can pass the how parameter.
  2. It breaks compatibility with previous versions. Checking the version before and conditioning on having at least 0.18.0 solves this.

For the month separator, my solution was to draw a bunch of lines with a bit of logic to figure out how to deal with the first day of the month. I also slightly modified were the month labels were placed because they didn't line up very well.

@martijnvermaat
Copy link
Owner

martijnvermaat commented Dec 7, 2016

@ma-schmidt Very nice, thanks!

Could you split the changes into:

  1. Those related to agg (Pandas >= 0.18)
  2. Month separator

Either as two commits in this PR (force-pushed) or a new PR, or as two separate PRs, anyway you like. This would make it much easier for me to review and merge.

It would also be cool if you could have a look at the failing tests.

@ma-schmidt
Copy link
Contributor Author

I don't know if what I did was what you wanted, but I have added two separate commits (08616e7 and 9009111).

The failing test is due to a slight change in the output png file. In other words, the test is failing when it is trying to compare the baseline image in tests/baseline/. This is coming from the month separator feature, not the fix to the pandas warning.

Unfortunately, I have no clue how to change these images (coming from an academic and scientific programming background, I don't know how to automatic tests work). Could you either tell me where to start or simply generate the new images yourself if you have time?

The slight changes I made to the image that broke the testing are twofold. First, I centered the x_axis labels a little bit better. The month separator made it clear that they were off since the tick was placed to the right of the 15th. The second change is also esthetic and is simply to add a margin of 0.3 around the image so that the black lines would not get cut out. When I was creating my own plot I found these tweaks were better looking, but I understand if you do not want to implement them in the package.

@ma-schmidt
Copy link
Contributor Author

Actually, I found a bug in the month separator feature and it will be easier to simply separate the changes in two separate PR. I am closing this one.

@ma-schmidt ma-schmidt closed this Dec 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants