Re: [matplotlib-devel] modification of update_path_extents?
Eric Firing wrote: > Mike, > > A bug was recently pointed out: axhline, axvline, axhspan, axvspan > mess up the ax.dataLim. I committed a quick fix for axhline and > axvline, but I don't think that what I did is a good solution, so > before doing anything for axhspan and axvspan I want to arrive at a > better strategy. > > What is needed is a clean way to specify that only the x or the y part > of ax.dataLim be updated when a line or patch (or potentially anything > else) is added. This is specifically for the case, as in *line, where > one axis is in data coordinates and the other is in normalized > coordinates--we don't want the latter to have any effect on the dataLim. > > This could be done in python in any of a variety of ways, but I > suspect that to be most consistent with the way the transforms code is > now written, relying on update_path_extends from _path.cpp, it might > make sense to append two boolean arguments to that cpp function, > "update_x" and "update_y", and use kwargs in Bbox.update_from_path and > siblings to set these, with default values of True. It seems we could do this without touching C at all. Just change update_from_path so it only updates certain coordinates in the bounding box based on the kwargs you propose. Sure, the C side will be keeping track of y bounds even when it doesn't have to, but I doubt that matters much compared to checking a flag in the inner loop. It will compute the bezier curves for both x and y anyway (without digging into Agg). It's hard to really estimate the performance impact, so I'm necessarily pushing for either option, but it may save having to update the C. > What do you think? If you agree to the _path.cpp change strategy, do > you prefer to do that yourself, or would you rather that I try it first? I probably won't have a chance to look at this today, so go ahead if you like. I'll shoot you a note later in the week if I have time... Cheers, Mike > > Any suggestions and/or contributions are welcome. > > Thanks. > > Eric -- Michael Droettboom Science Software Branch Operations and Engineering Division Space Telescope Science Institute Operated by AURA for NASA - This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world http://moblin-contest.org/redirect.php?banner_id=100&url=/ ___ Matplotlib-devel mailing list Matplotlib-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/matplotlib-devel
Re: [matplotlib-devel] modification of update_path_extents?
Michael Droettboom wrote: > > Eric Firing wrote: >> Mike, >> >> A bug was recently pointed out: axhline, axvline, axhspan, axvspan >> mess up the ax.dataLim. I committed a quick fix for axhline and >> axvline, but I don't think that what I did is a good solution, so >> before doing anything for axhspan and axvspan I want to arrive at a >> better strategy. >> >> What is needed is a clean way to specify that only the x or the y part >> of ax.dataLim be updated when a line or patch (or potentially anything >> else) is added. This is specifically for the case, as in *line, where >> one axis is in data coordinates and the other is in normalized >> coordinates--we don't want the latter to have any effect on the dataLim. >> >> This could be done in python in any of a variety of ways, but I >> suspect that to be most consistent with the way the transforms code is >> now written, relying on update_path_extends from _path.cpp, it might >> make sense to append two boolean arguments to that cpp function, >> "update_x" and "update_y", and use kwargs in Bbox.update_from_path and >> siblings to set these, with default values of True. > It seems we could do this without touching C at all. Just change > update_from_path so it only updates certain coordinates in the bounding > box based on the kwargs you propose. Sure, the C side will be keeping > track of y bounds even when it doesn't have to, but I doubt that matters > much compared to checking a flag in the inner loop. It will compute the > bezier curves for both x and y anyway (without digging into Agg). It's > hard to really estimate the performance impact, so I'm necessarily > pushing for either option, but it may save having to update the C. Mike, I was somehow thinking that update_path_extents was changing things in place--completely wrong. So yes, it is trivial to make the change at the python level, and that is definitely the place to do it. I will try to take care of it this evening. In poking around, however, I came up with a couple of questions. Neither is a blocker for what I need to do, but each might deserve a comment in the code, if nothing else. 1) in _path.cpp: void get_path_extents(PathIterator& path, const agg::trans_affine& trans, double* x0, double* y0, double* x1, double* y1, double* xm, double* ym) { typedef agg::conv_transform transformed_path_t; typedef agg::conv_curve curve_t; double x, y; unsigned code; transformed_path_t tpath(path, trans); curve_t curved_path(tpath); curved_path.rewind(0); while ((code = curved_path.vertex(&x, &y)) != agg::path_cmd_stop) { if ((code & agg::path_cmd_end_poly) == agg::path_cmd_end_poly) continue; /* if (MPL_notisfinite64(x) || MPL_notisfinite64(y)) continue; We should not need the above, because the path iterator should already be filtering out invalid values. */ if (x < *x0) *x0 = x; if (y < *y0) *y0 = y; if (x > *x1) *x1 = x; if (y > *y1) *y1 = y; if (x > 0.0 && x < *xm) *xm = x; if (y > 0.0 && y < *ym) *ym = y; } } In the last 2 lines, why are xm and ym being clipped at 0, when x0 and y0 are not? 2) It looks like update_path_extents throws away orientation by always returning x0 and y0 as the minima. Bbox.update_from_path is therefore doing the same. This doesn't hurt in present usage, since orientation is not needed for dataLim, but it seems a bit surprising, and worth a comment at least. Am I again missing something obvious? Eric - This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world http://moblin-contest.org/redirect.php?banner_id=100&url=/ ___ Matplotlib-devel mailing list Matplotlib-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/matplotlib-devel
Re: [matplotlib-devel] modification of update_path_extents?
Eric Firing wrote: > Michael Droettboom wrote: >> >> Eric Firing wrote: >>> Mike, >>> >>> A bug was recently pointed out: axhline, axvline, axhspan, axvspan >>> mess up the ax.dataLim. I committed a quick fix for axhline and >>> axvline, but I don't think that what I did is a good solution, so >>> before doing anything for axhspan and axvspan I want to arrive at a >>> better strategy. >>> >>> What is needed is a clean way to specify that only the x or the y >>> part of ax.dataLim be updated when a line or patch (or potentially >>> anything else) is added. This is specifically for the case, as in >>> *line, where one axis is in data coordinates and the other is in >>> normalized coordinates--we don't want the latter to have any effect >>> on the dataLim. >>> >>> This could be done in python in any of a variety of ways, but I >>> suspect that to be most consistent with the way the transforms code >>> is now written, relying on update_path_extends from _path.cpp, it >>> might make sense to append two boolean arguments to that cpp >>> function, "update_x" and "update_y", and use kwargs in >>> Bbox.update_from_path and siblings to set these, with default values >>> of True. >> It seems we could do this without touching C at all. Just change >> update_from_path so it only updates certain coordinates in the >> bounding box based on the kwargs you propose. Sure, the C side will >> be keeping track of y bounds even when it doesn't have to, but I >> doubt that matters much compared to checking a flag in the inner >> loop. It will compute the bezier curves for both x and y anyway >> (without digging into Agg). It's hard to really estimate the >> performance impact, so I'm necessarily pushing for either option, but >> it may save having to update the C. > > Mike, > > I was somehow thinking that update_path_extents was changing things in > place--completely wrong. So yes, it is trivial to make the change at > the python level, and that is definitely the place to do it. I will > try to take care of it this evening. > > In poking around, however, I came up with a couple of questions. > Neither is a blocker for what I need to do, but each might deserve a > comment in the code, if nothing else. > > 1) in _path.cpp: > > void get_path_extents(PathIterator& path, const agg::trans_affine& trans, > double* x0, double* y0, double* x1, double* y1, > double* xm, double* ym) > { > typedef agg::conv_transform transformed_path_t; > typedef agg::conv_curve curve_t; > double x, y; > unsigned code; > > transformed_path_t tpath(path, trans); > curve_t curved_path(tpath); > > curved_path.rewind(0); > > while ((code = curved_path.vertex(&x, &y)) != agg::path_cmd_stop) > { > if ((code & agg::path_cmd_end_poly) == agg::path_cmd_end_poly) > continue; > /* if (MPL_notisfinite64(x) || MPL_notisfinite64(y)) > continue; >We should not need the above, because the path iterator >should already be filtering out invalid values. > */ > if (x < *x0) *x0 = x; > if (y < *y0) *y0 = y; > if (x > *x1) *x1 = x; > if (y > *y1) *y1 = y; > if (x > 0.0 && x < *xm) *xm = x; > if (y > 0.0 && y < *ym) *ym = y; > } > } > > > In the last 2 lines, why are xm and ym being clipped at 0, when x0 and > y0 are not? xm and ym are the minimum positive values, used for log scales. Definitely worth a comment. > > 2) It looks like update_path_extents throws away orientation by always > returning x0 and y0 as the minima. Bbox.update_from_path is therefore > doing the same. This doesn't hurt in present usage, since orientation > is not needed for dataLim, but it seems a bit surprising, and worth a > comment at least. Am I again missing something obvious? > I think I'm missing something. (Choices in my code are always obvious to *me*, and any mistakes are invisible... ;) Are you suggesting that if a line has x decreasing then x0 > x1? What if it boomerangs? I think it's simplest to keep data limits in a consistent order. View limits are another issue, of course. Mike -- Michael Droettboom Science Software Branch Operations and Engineering Division Space Telescope Science Institute Operated by AURA for NASA - This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world http://moblin-contest.org/redirect.php?banner_id=100&url=/ ___ Matplotlib-devel mailing list Matplotlib-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/matplotlib-devel
Re: [matplotlib-devel] modification of update_path_extents?
>> if (x > 0.0 && x < *xm) *xm = x; >> if (y > 0.0 && y < *ym) *ym = y; >> } >> } >> >> >> In the last 2 lines, why are xm and ym being clipped at 0, when x0 and >> y0 are not? > xm and ym are the minimum positive values, used for log scales. > Definitely worth a comment. I will add one. >> >> 2) It looks like update_path_extents throws away orientation by always >> returning x0 and y0 as the minima. Bbox.update_from_path is therefore >> doing the same. This doesn't hurt in present usage, since orientation >> is not needed for dataLim, but it seems a bit surprising, and worth a >> comment at least. Am I again missing something obvious? >> > I think I'm missing something. (Choices in my code are always obvious > to *me*, and any mistakes are invisible... ;) Are you suggesting that > if a line has x decreasing then x0 > x1? What if it boomerangs? I > think it's simplest to keep data limits in a consistent order. View > limits are another issue, of course. I think I will add a comment. The only "problem" at present is that update_from_path, taken as a perfectly general Bbox method, is doing a bit more than is obvious from its name or even from the code, and this places at least a theoretical restriction on its potential use. For example, if someone looked at the method and thought he or she could use it to directly update a viewLim, the result would not be as expected. Eric > > Mike > - This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world http://moblin-contest.org/redirect.php?banner_id=100&url=/ ___ Matplotlib-devel mailing list Matplotlib-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/matplotlib-devel