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

COMPAT: use tkagg backend on PyPy #9356

Merged
merged 12 commits into from Jan 12, 2018
Copy path View file
@@ -13,6 +13,8 @@
#include <cstdio>
#include <sstream>
#include <mplutils.h> // uint8_t on old MSC_VER
// Include our own excerpts from the Tcl / Tk headers
#include "_tkmini.h"
@@ -101,10 +103,10 @@ static int PyAggImagePhoto(ClientData clientdata, Tcl_Interp *interp, int
if (has_bbox) {
int srcstride = wdata * 4;
destx = x1;
desty = hdata - y2;
destwidth = x2 - x1;
destheight = y2 - y1;
destx = (int)x1;

This comment has been minimized.

@anntzer

anntzer Nov 24, 2017

Contributor

Shouldn't BBOX_FORMAT just be 4 integers and just use integers all along? (not sure if tk would have the bad idea to always cast things to float)

Edit: Actually I guess not, what should really happen is that we should explicitly cast on the caller's side but this can just go as it is for now...

This comment has been minimized.

@mattip

mattip Nov 24, 2017

Contributor

The call sequence is backend.blit(image, bbox) -> tkagg.py:blit(image, bbox) -> PyAggImagePhoto(as_string(image, bbox)) so we cannot change the root backend.blit call. AFAICT the choice is:

  • convert early in tkagg.py:blit
  • convert later in PyAggImagePhoto

I usually prefer converting as late as possible to postpone rounding and allow future refactoring with floats to be as painless as possible, but will flow with whatever is matplotlib best practices

This comment has been minimized.

@anntzer

anntzer Nov 24, 2017

Contributor

I think we can leave it as it is for now, we were using floats to start with and we can change that later.

desty = (int)(hdata - y2);
destwidth = (int)(x2 - x1);
destheight = (int)(y2 - y1);
deststride = 4 * destwidth;
destbuffer = new uint8_t[deststride * destheight];

This comment has been minimized.

@anntzer

anntzer Nov 23, 2017

Contributor

any reason for the change?

This comment has been minimized.

@mattip

mattip Nov 23, 2017

Contributor

now it no longer uses #include py_converters.h which supplied the agg:int8u

This comment has been minimized.

@tacaswell

tacaswell Dec 11, 2017

Member

Why change away from the agg types here?

This comment has been minimized.

@mattip

mattip Dec 11, 2017

Contributor

Previously there was a conversion from a PyArrayObject * (in other words ndarray) to a data pointer, width, height, etc via py_converters. Now I pass that information directly from python. I no longer need the "py_converters.cpp" and ""py_converters.h" since there is no use of ndarray in the c++ code. Should I leave the include for this one type definition?

This comment has been minimized.

@tacaswell

tacaswell Dec 11, 2017

Member

I am concerned about the corner case where unint8_t and agg:int8u are different underlying types. Not sure how likely that actually is, hence leaning towards being conservative about changing it.

This comment has been minimized.

@anntzer

anntzer Dec 12, 2017

Contributor

I wonder what different types you think these can be.
I would also bet that there must be parts of the code where we assume that this is interconvertible with np.uint8, or that this fits in a single byte... doesn't leave you a lot of options!

ProTip! Use n and p to navigate between commits in a pull request.