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

geom: improvements to the base class #175

Closed
jankatins opened this issue Jan 22, 2014 · 5 comments
Closed

geom: improvements to the base class #175

jankatins opened this issue Jan 22, 2014 · 5 comments

Comments

@jankatins
Copy link
Contributor

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:

  • differentiate beween aes mapping (via geom(aes(...))), aes setting (via geom(color=red)) and "other" stuff passed in (see stat_function)
  • a proper layer builder (code duplication)
  • a proper validator for required aes and other stuff (code duplication)
  • get this somehow displayed in the docs (check how pandas does it with their config system and docs (class decorator, which appends the info to the docstring)
@jankatins
Copy link
Contributor Author

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
Copy link

has2k1 commented Mar 7, 2014

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
Copy link

has2k1 commented Mar 10, 2014

@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
Copy link
Contributor

glamp commented Mar 10, 2014

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//issues/175#issuecomment-37162792
.

This was referenced Mar 11, 2014
@jankatins
Copy link
Contributor Author

This is merged in #252

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

No branches or pull requests

3 participants