yhat / ggpy

ggplot port for python
http://yhat.github.io/ggpy/
BSD 2-Clause "Simplified" License
3.7k stars 573 forks source link

geom: improvements to the base class #175

Closed jankatins closed 10 years ago

jankatins commented 10 years ago

Currently all arguments to subclasses to geom end up as aes or manual_aes, but sometimes this is not enough (see stat_function). There is also a lot of code duplication in each plot_layer function (checking, layer building)

It would be nice to improve the abstract geom class:

jankatins commented 10 years ago

There is an awful lot of code duplication (checking if all required aes are there, updating the layer dict,...) which could all go into a default method in geom which would then either call the 'plot_layer()' method ore could be called from there.

has2k1 commented 10 years ago

I think the lack of an explicit distinction between the mappings (1st point) makes bugs such as #232 creep in. It is obvious we need the three distinctions, ggplot_mapping, geom_mapping, geom_other_args and though ggplot2 allows for extra parameters to ggplot which get passed to the layer it is not apparent that we need this one yet.

Incorporating the distinction may have to wait until after #221, but before then I can get some of the code duplication (2nd and 3rd points) out of the way.

has2k1 commented 10 years ago

@glamp, I am working on the 2nd and 3rd points mentioned by @JanSchulz above, it involves;

  1. Create geom.plot_layer , do preprocessing and call geom_xxx.plot
  2. Cross-check the ggplot2 API requirements for the geoms and fill out the valid & required aesthetics and the other arguments to the geoms.
  3. Remove the duplicate code in geom_xxx.plot, (code that did the preprocessing now taken care of in geom.plot_layer).

Currently some geoms like geom_hline can take and make use of extra arguments to potentially do more than ggplot2. For geom_hline (horizontal line), in ggplot2 it is always across the entire plot, however we currently have arguments xmin and xmax for partial horizontal lines. Should we have arguments and functionality beyond the ggplot2 API?

glamp commented 10 years ago

I definitely don't think it hurts. While the goal is to keep consistent with the ggplot2 api, realistically I don't think that's going to happen.

On Mon, Mar 10, 2014 at 4:57 AM, has2k1 notifications@github.com wrote:

@glamp https://github.com/glamp, I am working on the 2nd and 3rd points mentioned by @JanSchulz https://github.com/JanSchulz above, it involves;

  1. Create geom.plot_layer , do preprocessing and call geom_xxx.plot
  2. Cross-check the ggplot2 API requirements for the geoms and fill out the valid & required aesthetics and the other arguments to the geoms.
  3. Remove the duplicate code in geom_xxx.plot, (code that did the preprocessing now taken care of in geom.plot_layer).

Currently some geoms like geom_hline can take and make use of extra arguments to potentially do more than ggplot2. For geom_hline (horizontal line), in ggplot2 it is always across the entire plot, however we currently have arguments xmin and xmax for partial horizontal lines. Should we have arguments and functionality beyond the ggplot2 API?

Reply to this email directly or view it on GitHubhttps://github.com/yhat/ggplot/issues/175#issuecomment-37162792 .

jankatins commented 10 years ago

This is merged in #252