From: Steve S. <ste...@gm...> - 2018-11-20 18:15:47
Attachments:
plbox.c
|
(re-sending from my list subscription address…) Hi Plplot We have been pretty quiet, happily employing plplot in our QSAS software. We recently decided to update our plplot version, which we don’t do regurarly since it requires a few tweaks to work with/embed in our system. We (meaning my colleague and software person Tony Allen – in cc) have discovered a bug in the handling of exponential label placement. While we haven’t prepared a minimal example, the exchange below describes the symptoms. The attached plbox.c includes Tony’s correction. Best wishes Steve From: Allen, Anthony J <a....@im... <mailto:a....@im...> > Sent: Monday, November 19, 2018 10:55 AM To: Steven Schwartz <Ste...@Co... <mailto:Ste...@Co...> > Subject: Re: Exponential labels OK, this is fixed, but I had to modify plbox.c (attached). They had kindly added a specific mod for QSAS (one of Alban’s that I noticed didn’t need doing in the latest version) but they missed out the else{} part, so it fell through to do their placement as well! Can you forward this to them? Thanks, Tony On 15 Nov 2018, at 21:26, Steven Schwartz <Ste...@Co... <mailto:Ste...@Co...> > wrote: Hi Tony, Make the plot from the attached session. If you look at the labels in, e.g,. the panel in the top frame you’ll notice two things: 1 There are TWO y-axis exponential labels: one at the top, and one below the bottom left. 2 I’ve set the panel’s exponential label horizontal displacement to be -1. You can see that the top label has been moved left while the bottom label has been moved down, by comparison to the bottom frame which has a panel exponential label horizontal displacement of 0.0 I think the bottom left exponential label shouldn’t be there at all. The horizontal axis has its own single exponential label, bottom right, that is properly controlled in the Frame tab. And if there’s some reason why it needs to be there, it should move horizontally with a horizontal offset value. ? Thanks Steve Steve Schwartz Research Associate Laboratory for Atmospheric and Space Physics University of Colorado Boulder 3665 Discovery Drive, UCB 600, Boulder, CO 80305, USA <mailto:ste...@la...> ste...@la... <ExpLabelPosition.qss.zip> |
From: Alan W. I. <Ala...@gm...> - 2018-11-22 22:49:51
|
On 2018-11-20 11:15-0700 Steve Schwartz wrote: > Hi Plplot > > > > We have been pretty quiet, happily employing plplot in our QSAS software. We recently decided to update our plplot version, which we don’t do regurarly since it requires a few tweaks to work with/embed in our system. > > > > We (meaning my colleague and software person Tony Allen – in cc) have discovered a bug in the handling of exponential label placement. While we haven’t prepared a minimal example, the exchange below describes the symptoms. The attached plbox.c includes Tony’s correction. Hi Steve and Tony: @Steve: Thanks for passing along Tony's fix for a bug in PLplot that he noticed. @Tony: Most of this is addressed to you. But before you respond please take a few minutes to subscribe to plplot-devel so everybody subscribed to that list (not just those you directly e-mail) and those looking at the list archive later can see what you say. (See <https://sourceforge.net/projects/plplot/lists/plplot-devel> for the simple subscription directions and remember to always use your subscription address as the return address for your posts to this list or otherwise your message will be lost to everybody other than the list manager [who happens to be me].) I took your version of plbox.c (as forwarded by Steve), changed from Windows to Unix line endings (git assumes native line endings, e.g., Windows on Windows and Unix on Linux), commented out some debugging print statements, changed one comment reference from "qsas" to "QSAS" to be consistent with the same comment in another routine, and styled that result. So after those further changes here are the differences with the PLplot HEAD version (from the "git diff --ignore-all-space --unified=10" command with that last-parameter used to give lots of context). -------------------------------------- diff --git a/src/plbox.c b/src/plbox.c index b2b25840f..01c5aee7b 100644 --- a/src/plbox.c +++ b/src/plbox.c @@ -1470,21 +1470,21 @@ label_box( PLCHAR_VECTOR xopt, PLFLT xtick1, PLCHAR_VECTOR yopt, PLFLT ytick1 ) ldy = plP_stsearch( yopt, 'd' ); lfy = plP_stsearch( yopt, 'f' ); liy = plP_stsearch( yopt, 'i' ); lly = plP_stsearch( yopt, 'l' ); lmy = plP_stsearch( yopt, 'm' ); lny = plP_stsearch( yopt, 'n' ); lty = plP_stsearch( yopt, 't' ); lvy = plP_stsearch( yopt, 'v' ); loy = plP_stsearch( yopt, 'o' ); lxy = plP_stsearch( yopt, 'x' ); - + //printf( "=== yopt %d %d %d \n", lny, lty, lvy ); plP_xgvpw( &vpwxmin, &vpwxmax, &vpwymin, &vpwymax ); // vpwxmi always numerically less than vpwxma, and // similarly for vpwymi vpwxmi = ( vpwxmax > vpwxmin ) ? vpwxmin : vpwxmax; vpwxma = ( vpwxmax > vpwxmin ) ? vpwxmax : vpwxmin; vpwymi = ( vpwymax > vpwymin ) ? vpwymin : vpwymax; vpwyma = ( vpwymax > vpwymin ) ? vpwymax : vpwymin; // Write label(s) for horizontal axes. if ( ( lmx || lnx ) && ( ltx || lxx ) ) @@ -1819,21 +1819,24 @@ label_box( PLCHAR_VECTOR xopt, PLFLT xtick1, PLCHAR_VECTOR yopt, PLFLT ytick1 ) // Write separate exponential label if mode = 1. if ( !lly && !ldy && !loy && ymode ) { snprintf( string, STRING_LEN, "(x10%su%d%sd)", esc_string, (int) yscale, esc_string ); if ( custom_exponent_placement ) { height = ( (PLLabelDefaults *) plsc->label_data )->exp_label_disp; pos = ( (PLLabelDefaults *) plsc->label_data )->exp_label_pos; just = ( (PLLabelDefaults *) plsc->label_data )->exp_label_just; + plmtex( "t", height, pos, just, string ); // added for QSAS } + else + { if ( lvy ) { offset = 0.1; // more space to clear labels in "v" mode } else { offset = 0.02; } // Left axis exponent if ( lny ) @@ -1907,20 +1910,21 @@ label_box( PLCHAR_VECTOR xopt, PLFLT xtick1, PLCHAR_VECTOR yopt, PLFLT ytick1 ) } else { plmtex( "r", height, pos, just, string ); } } } } } } +} //-------------------------------------------------------------------------- // void label_box_custom() // // Writes numeric labels on side(s) of box in custom locations //-------------------------------------------------------------------------- void label_box_custom( PLCHAR_VECTOR xopt, PLINT n_xticks, PLFLT_VECTOR xticks, PLCHAR_VECTOR yopt, PLINT n_yticks, PLFLT_VECTOR yticks ) { @@ -1931,21 +1935,21 @@ label_box_custom( PLCHAR_VECTOR xopt, PLINT n_xticks, PLFLT_VECTOR xticks, PLCHA PLFLT vpwxmin, vpwxmax, vpwymin, vpwymax; PLFLT tn, offset; PLCHAR_VECTOR timefmt; PLINT i; PLINT xdigmax, xdigits, xdigmax_old, xdigits_old; PLINT ydigmax, ydigits, ydigmax_old, ydigits_old; PLINT lxmin, lxmax, lymin, lymax; PLINT pxmin, pxmax, pymin, pymax; PLFLT default_mm, char_height_mm, height_mm; PLFLT string_length_mm = 0.0, pos_mm = 0.0; - + //printf( "=== custom\n" ); // Assume label data is for placement of exponents if no custom // label function is provided. PLBOOL custom_exponent_placement = !plsc->label_func && plsc->label_data; // pos, height, and just are unnecessarily set to quiet // -O3 -Wuninitialized warnings that are obvious false alarms from // the clarity of the code associated with the true or false // result for custom_exponent_placement. PLFLT pos = 0.0, height = 0.0, just = 0.0; PLCHAR_VECTOR esc_string = plgesc_string(); @@ -2384,20 +2388,21 @@ label_box_custom( PLCHAR_VECTOR xopt, PLINT n_xticks, PLFLT_VECTOR xticks, PLCHA // Write separate exponential label if mode = 1. if ( !lly && !ldy && !loy && ymode ) { snprintf( string, STRING_LEN, "(x10%su%d%sd)", esc_string, (int) yscale, esc_string ); if ( custom_exponent_placement ) { height = ( (PLLabelDefaults *) plsc->label_data )->exp_label_disp; pos = ( (PLLabelDefaults *) plsc->label_data )->exp_label_pos; just = ( (PLLabelDefaults *) plsc->label_data )->exp_label_just; + plmtex( "t", height, pos, just, string ); //added for QSAS } if ( lvy ) { offset = 0.1; // more space to clear labels in "v" mode } else { offset = 0.02; } // Left axis exponent. -------------------------------------- For the record, label_box_custom is a private function that is only called by the static function draw_box in src/pllegend.c, which in turn is only called by c_plcolorbar within that source file. So label_box_custom is a misnomer, and I plan (after the current release is done) to address that issue by changing that name to label_box_colorbar. But from now on in this e-mail I will refer to that function with its present name of label_box_custom. (@ Everybody: Note that Tony's fix is limited to changes when custom_exponent_placement is true, i.e., when there is no custom label function but (custom?) label data are still provided to change the placement of the exponential label.) @Tony: We really do need a simple test case for both the revised label_box and label_box_custom functions. Could you provide that? I suggest that simple example doesn't need any data to plot, but it does need to contain a labelled box and a simple colorbar (see example 16 for an example of a simple colorbar) to exercise both label_box and label_box_custom. And, of course, both the box and colorbar should have exponential labels and appropriate label data should be provided in each case to exercise the cases for both functions where custom_exponent_placement is true. It has been a long time since I looked at plbox.c, but my impression now is label_box_custom is quite similar to label_box code so I am pretty sure label_box_custom badly needs documentation about exactly how and why it is changed from label_box for the special needs of plcolorbar. So providing such documentation is on my post-release agenda, but in the absence of such documentation I am frankly currently ignorant of the designed differences between label_box and label_box_custom. However, with that caveat I don't understand why your fix is so different in the two separate functions. For example, prior to your fix label_box and label_box_custom had very similar custom_exponent_placement logic, but now they are quite different (e.g., the above added "else" in label_box that skips a huge amount of code is not present in label_box_custom). Did you really mean to have such quite different logic in those two cases? Even if the present fix works, it would be best for the logic in those two functions to be implemented as similarly as possible. Assuming your reply to my questions includes another revised version of plbox.c, I don't care about the line endings and whitespace styling changes I made since those are easy to do again. However, please do incorporate my further changes (commenting out of the print functions and the qsas ==> QSAS change) in your revised code. N.B. I am happy to take a quick break (as now) from getting out the 5.14.0 release to discuss these changes with you, but we are in deep freeze now (i.e., it is obviously much too late in the present release cycle to get these or virtually any other code change into that release). But once you provide a good simple test example to make sure all is well with these changes, it would be ideal to get these changes accepted for PLplot early in the release cycle for 5.15.0. Best wishes, Alan __________________________ Alan W. Irwin Programming affiliations with the FreeEOS equation-of-state implementation for stellar interiors (freeeos.sf.net); the Time Ephemerides project (timeephem.sf.net); PLplot scientific plotting software package (plplot.sf.net); the libLASi project (unifont.org/lasi); the Loads of Linux Links project (loll.sf.net); and the Linux Brochure Project (lbproject.sf.net). __________________________ Linux-powered Science __________________________ |
From: Allen, A. J <a....@im...> - 2018-11-23 11:40:20
|
Hi Alan, firstly, my apologies for leaving in some debug output. The changes appear as I would have expected with just the else { } component and a couple of plmtex() calls. These were originally implemented by Alban Rochel in our copy so I don’t know the full history - he just left us instructions on how to tweek plplot for QSAS. I can’t actually see label_box_custom() being called anywhere in the latest version, (either by you or us) just in the earlier version we used previously. I’ll have to look at how we label colour bars, but I’m pretty certain we draw our own anyway! I’m mostly retired now, I only work 1 day a week, but I’ll see if I can create a test rig that does not involve running QSAS but uses our calls. Regards, Tony > > Hi Steve and Tony: > > @Steve: > Thanks for passing along Tony's fix for a bug in PLplot that he noticed. > > @Tony: > > Most of this is addressed to you. But before you respond please take > a few minutes to subscribe to plplot-devel so everybody subscribed to > that list (not just those you directly e-mail) and those looking at > the list archive later can see what you say. (See > <https://sourceforge.net/projects/plplot/lists/plplot-devel> for the > simple subscription directions and remember to always use your > subscription address as the return address for your posts to this list > or otherwise your message will be lost to everybody other than the > list manager [who happens to be me].) > > I took your version of plbox.c (as forwarded by Steve), changed from > Windows to Unix line endings (git assumes native line endings, e.g., > Windows on Windows and Unix on Linux), commented out some debugging > print statements, changed one comment reference from "qsas" to "QSAS" > to be consistent with the same comment in another routine, and styled > that result. So after those further changes here are the differences > with the PLplot HEAD version (from the "git diff --ignore-all-space > --unified=10" command with that last-parameter used to give lots of > context). > > -------------------------------------- > diff --git a/src/plbox.c b/src/plbox.c > index b2b25840f..01c5aee7b 100644 > --- a/src/plbox.c > +++ b/src/plbox.c > @@ -1470,21 +1470,21 @@ label_box( PLCHAR_VECTOR xopt, PLFLT xtick1, PLCHAR_VECTOR yopt, PLFLT ytick1 ) > ldy = plP_stsearch( yopt, 'd' ); > lfy = plP_stsearch( yopt, 'f' ); > liy = plP_stsearch( yopt, 'i' ); > lly = plP_stsearch( yopt, 'l' ); > lmy = plP_stsearch( yopt, 'm' ); > lny = plP_stsearch( yopt, 'n' ); > lty = plP_stsearch( yopt, 't' ); > lvy = plP_stsearch( yopt, 'v' ); > loy = plP_stsearch( yopt, 'o' ); > lxy = plP_stsearch( yopt, 'x' ); > - > + //printf( "=== yopt %d %d %d \n", lny, lty, lvy ); > plP_xgvpw( &vpwxmin, &vpwxmax, &vpwymin, &vpwymax ); > // vpwxmi always numerically less than vpwxma, and > // similarly for vpwymi > vpwxmi = ( vpwxmax > vpwxmin ) ? vpwxmin : vpwxmax; > vpwxma = ( vpwxmax > vpwxmin ) ? vpwxmax : vpwxmin; > vpwymi = ( vpwymax > vpwymin ) ? vpwymin : vpwymax; > vpwyma = ( vpwymax > vpwymin ) ? vpwymax : vpwymin; > > // Write label(s) for horizontal axes. > if ( ( lmx || lnx ) && ( ltx || lxx ) ) > @@ -1819,21 +1819,24 @@ label_box( PLCHAR_VECTOR xopt, PLFLT xtick1, PLCHAR_VECTOR yopt, PLFLT ytick1 ) > // Write separate exponential label if mode = 1. > > if ( !lly && !ldy && !loy && ymode ) > { > snprintf( string, STRING_LEN, "(x10%su%d%sd)", esc_string, (int) yscale, esc_string ); > if ( custom_exponent_placement ) > { > height = ( (PLLabelDefaults *) plsc->label_data )->exp_label_disp; > pos = ( (PLLabelDefaults *) plsc->label_data )->exp_label_pos; > just = ( (PLLabelDefaults *) plsc->label_data )->exp_label_just; > + plmtex( "t", height, pos, just, string ); // added for QSAS > } > + else > + { > if ( lvy ) > { > offset = 0.1; // more space to clear labels in "v" mode > } > else > { > offset = 0.02; > } > // Left axis exponent > if ( lny ) > @@ -1907,20 +1910,21 @@ label_box( PLCHAR_VECTOR xopt, PLFLT xtick1, PLCHAR_VECTOR yopt, PLFLT ytick1 ) > } > else > { > plmtex( "r", height, pos, just, string ); > } > } > } > } > } > } > +} > > //-------------------------------------------------------------------------- > // void label_box_custom() > // > // Writes numeric labels on side(s) of box in custom locations > //-------------------------------------------------------------------------- > > void > label_box_custom( PLCHAR_VECTOR xopt, PLINT n_xticks, PLFLT_VECTOR xticks, PLCHAR_VECTOR yopt, PLINT n_yticks, PLFLT_VECTOR yticks ) > { > @@ -1931,21 +1935,21 @@ label_box_custom( PLCHAR_VECTOR xopt, PLINT n_xticks, PLFLT_VECTOR xticks, PLCHA > PLFLT vpwxmin, vpwxmax, vpwymin, vpwymax; > PLFLT tn, offset; > PLCHAR_VECTOR timefmt; > PLINT i; > PLINT xdigmax, xdigits, xdigmax_old, xdigits_old; > PLINT ydigmax, ydigits, ydigmax_old, ydigits_old; > PLINT lxmin, lxmax, lymin, lymax; > PLINT pxmin, pxmax, pymin, pymax; > PLFLT default_mm, char_height_mm, height_mm; > PLFLT string_length_mm = 0.0, pos_mm = 0.0; > - > + //printf( "=== custom\n" ); > // Assume label data is for placement of exponents if no custom > // label function is provided. > PLBOOL custom_exponent_placement = !plsc->label_func && plsc->label_data; > > // pos, height, and just are unnecessarily set to quiet > // -O3 -Wuninitialized warnings that are obvious false alarms from > // the clarity of the code associated with the true or false > // result for custom_exponent_placement. > PLFLT pos = 0.0, height = 0.0, just = 0.0; > PLCHAR_VECTOR esc_string = plgesc_string(); > @@ -2384,20 +2388,21 @@ label_box_custom( PLCHAR_VECTOR xopt, PLINT n_xticks, PLFLT_VECTOR xticks, PLCHA > // Write separate exponential label if mode = 1. > > if ( !lly && !ldy && !loy && ymode ) > { > snprintf( string, STRING_LEN, "(x10%su%d%sd)", esc_string, (int) yscale, esc_string ); > if ( custom_exponent_placement ) > { > height = ( (PLLabelDefaults *) plsc->label_data )->exp_label_disp; > pos = ( (PLLabelDefaults *) plsc->label_data )->exp_label_pos; > just = ( (PLLabelDefaults *) plsc->label_data )->exp_label_just; > + plmtex( "t", height, pos, just, string ); //added for QSAS > } > if ( lvy ) > { > offset = 0.1; // more space to clear labels in "v" mode > } > else > { > offset = 0.02; > } > // Left axis exponent. > -------------------------------------- > > For the record, label_box_custom is a private function that is only > called by the static function draw_box in src/pllegend.c, which in > turn is only called by c_plcolorbar within that source file. So > label_box_custom is a misnomer, and I plan (after the current release > is done) to address that issue by changing that name to > label_box_colorbar. But from now on in this e-mail I will refer to > that function with its present name of label_box_custom. > > (@ Everybody: Note that Tony's fix is limited to changes when custom_exponent_placement > is true, i.e., when there is no custom label function but (custom?) label data > are still provided to change the placement of the exponential label.) > > @Tony: > > We really do need a simple test case for both the revised label_box and label_box_custom > functions. Could you provide that? I suggest that simple example doesn't need any > data to plot, but it does need to contain a labelled box and a simple colorbar > (see example 16 for an example of a simple colorbar) to exercise both label_box and > label_box_custom. And, of course, both the box and colorbar should have exponential > labels and appropriate label data should be provided in each case to exercise > the cases for both functions where custom_exponent_placement is true. > > It has been a long time since I looked at plbox.c, but my impression > now is label_box_custom is quite similar to label_box code so I am > pretty sure label_box_custom badly needs documentation about exactly > how and why it is changed from label_box for the special needs of > plcolorbar. So providing such documentation is on my post-release > agenda, but in the absence of such documentation I am frankly > currently ignorant of the designed differences between label_box and > label_box_custom. However, with that caveat I don't understand why > your fix is so different in the two separate functions. > > For example, prior to your fix label_box and label_box_custom had very > similar custom_exponent_placement logic, but now they are quite different > (e.g., the above added "else" in label_box that skips a huge amount > of code is not present in label_box_custom). Did you really mean > to have such quite different logic in those two cases? Even if > the present fix works, it would be best for the logic in those two > functions to be implemented as similarly as possible. > > Assuming your reply to my questions includes another revised version > of plbox.c, I don't care about the line endings and whitespace styling > changes I made since those are easy to do again. However, please do > incorporate my further changes (commenting out of the print functions > and the qsas ==> QSAS change) in your revised code. > > N.B. I am happy to take a quick break (as now) from getting out the > 5.14.0 release to discuss these changes with you, but we are in deep > freeze now (i.e., it is obviously much too late in the present release > cycle to get these or virtually any other code change into that > release). But once you provide a good simple test example to make > sure all is well with these changes, it would be ideal to get these > changes accepted for PLplot early in the release cycle for 5.15.0. > > Best wishes, > > Alan > __________________________ > Alan W. Irwin > > Programming affiliations with the FreeEOS equation-of-state > implementation for stellar interiors (freeeos.sf.net); the Time > Ephemerides project (timeephem.sf.net); PLplot scientific plotting > software package (plplot.sf.net); the libLASi project > (unifont.org/lasi); the Loads of Linux Links project (loll.sf.net); > and the Linux Brochure Project (lbproject.sf.net). > __________________________ > > Linux-powered Science > __________________________ |
From: Allen, A. J <a....@im...> - 2018-11-23 12:11:33
|
Alan, just to confirm, QSAS does not use your colour bar code, nor does it use label_box_custom(). Alban wrote his own colour bar using plbox(). This means any change we made to label_box_custom is historical and nugatory, and only the changes to plbox() are relevant ( the else{ } ). In fact we do not include pllegend.c in our version of plplot at all, which is why I did not see label_box_custom() being called. All the best Tony |
From: Alan W. I. <Ala...@gm...> - 2018-11-23 22:53:35
|
On 2018-11-23 12:11-0000 Allen, Anthony J wrote: > Alan, > > just to confirm, QSAS does not use your colour bar code, nor does it > use label_box_custom(). Alban wrote his own colour bar using plbox(). > > This means any change we made to label_box_custom is historical and nugatory, and only the > changes to plbox() are relevant ( the else{ } ). > > In fact we do not include pllegend.c in our version of plplot at all, which is why I did > not see label_box_custom() being called. Hi Tony: Of course, upstream does use both label_box and label_box_custom. So from that upstream perspective, I believe the plan should be to (1) implement the test case I suggested (a simple example that plots both a box and a color bar where both require exponential labels with different custom locations for each); and (2) assuming that simple example demonstrates the bug you discovered, then fix the bug in a consistent way for both label_box and label_box_custom. I have put this plan on my ToDo list and will keep you informed of my progress once I start working on this topic (likely early in 2019). However, if you decide to work along the lines of that upstream plan yourself before I can get to it, please keep me informed of your progress as well. Alan __________________________ Alan W. Irwin Programming affiliations with the FreeEOS equation-of-state implementation for stellar interiors (freeeos.sf.net); the Time Ephemerides project (timeephem.sf.net); PLplot scientific plotting software package (plplot.sf.net); the libLASi project (unifont.org/lasi); the Loads of Linux Links project (loll.sf.net); and the Linux Brochure Project (lbproject.sf.net). __________________________ Linux-powered Science __________________________ |