search

martijnvermaat / calmap

Calendar heatmaps from Pandas time series data -- See https://github.com/MarvinT/calmap/ for the maintained version
https://pythonhosted.org/calmap
MIT License
211 stars 61 forks source link

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

Closed ma-schmidt closed 7 years ago

ma-schmidt commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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.