Re: [PATCH] boxplot: introduce a default factor

[sfeam <sf...@us...>]

On Saturday, 03 January 2015 06:25:29 PM sfeam wrote:
> On Saturday, 03 January 2015 12:35:27 PM Jouke Witteveen wrote:
> > When a row in the datafile contains no factor, its y-value is stored in
> > place of the factor identifier, leading to ambiguities.
> > If the datafile contains
> > 0	0	0	foo
> > 0	1
> > The second row will be marked as part of the 'foo' boxplot.
> > 
> > This patch fixes that issue.
> > ---
> > diff -Naur gnuplot-5.0.0.orig/src/gadgets.h gnuplot-5.0.0/src/gadgets.h
> > --- gnuplot-5.0.0.orig/src/gadgets.h	2015-01-02 22:06:25.488402227 +0100
> > +++ gnuplot-5.0.0/src/gadgets.h	2015-01-02 22:06:25.458403577 +0100
> > @@ -279,6 +279,8 @@
> >  	BOXPLOT_FACTOR_LABELS_X2
> >  } t_boxplot_factor_labels;
> >  
> > +#define DEFAULT_BOXPLOT_FACTOR -1
> > +
> >  typedef struct boxplot_style {
> >      int limit_type;	/* 0 = multiple of interquartile 1 = fraction of points */
> >      double limit_value;
> > diff -Naur gnuplot-5.0.0.orig/src/plot2d.c gnuplot-5.0.0/src/plot2d.c
> > --- gnuplot-5.0.0.orig/src/plot2d.c	2015-01-02 22:06:25.488402227 +0100
> > +++ gnuplot-5.0.0/src/plot2d.c	2015-01-02 22:08:57.411693594 +0100
> > @@ -910,19 +910,22 @@
> >  		store2d_point(current_plot, i++, v[0], v[1], v[0], v[0],
> >  			      v[1] - v[2], v[1] + v[2], -1.0);
> >  	    } else {
> > -		double w;
> > +		double ylow, w;
> >  		if (current_plot->plot_style == CANDLESTICKS
> >  		    || current_plot->plot_style == FINANCEBARS) {
> >  		    int_warn(storetoken, "This plot style does not work with 1 or 2 cols. Setting to points");
> >  		    current_plot->plot_style = POINTSTYLE;
> > -		}
> > +		} else if (current_plot->plot_style == BOXPLOT)
> > +		    ylow = DEFAULT_BOXPLOT_FACTOR;
> > +		else
> > +		    ylow = v[1];
> >  		if (current_plot->plot_smooth == SMOOTH_ACSPLINES)
> >  		    w = 1.0;	/* Unit weights */
> >  		else
> >  		    w = -1.0;	/* Auto-width boxes in some styles */
> >  		/* Set x/y high/low to exactly [x,y] */
> >  		store2d_point(current_plot, i++, v[0], v[1], 
> > -						 v[0], v[0], v[1], v[1], w);
> > +						 v[0], v[0], ylow, v[1], w);
> >  	    }
> >  	    break;
> >  
> > @@ -1004,7 +1007,7 @@
> >  
> >  		case BOXPLOT:	/* x, y, width */
> >  		    store2d_point(current_plot, i++, v[0], v[1], v[0]-v[2]/2., v[0]+v[2]/2.,
> > -		    		  v[1], v[1], v[2]);
> > +				  DEFAULT_BOXPLOT_FACTOR, v[1], v[2]);
> >  		    break;
> >  
> >  #ifdef EAM_OBJECTS
> > @@ -1513,7 +1516,7 @@
> >      /* This can happen if the user specifies a non-existent column:
> >       * fall back to single-boxplot mode */
> >      if (!string)
> > -	return 0;
> > +	return DEFAULT_BOXPLOT_FACTOR;
> >  
> >      /* Remove the trailing garbage, quotes etc. from the string */ 
> >      trimmed_string = df_parse_string_field(string);
> 
> Bug #2
> I agree there may be a bug here, but I can't reproduce exactly
> what you describe.  Could you please provide a full example with
> both the input and the command used to plot?
> 
> If I understand the problem case correctly, a missing column in 
> the input data is a failure in its own right and the point should 
> be discarded before ever reaching your new code.
> The actual result seems to depend on additional factors like 
> "set datafile separator". But in cases where the point is not 
> discarded I don't see it incorrectly added to some other category
>  - instead the plot contains an extra boxplot column with a blank
> label into which the problem points are placed. 
> That's arguably a correct result, although it's not what I expected.

I've applied this patch with 2 modifications.
1) It handles the case of the factor column being present but empty
   (e.g. in a *.csv file) as well as the case of the column being
   missing altogether.
2) The factor is now stored in the z/w/misc slot of the point structure
   rather than misusing ylow.  For the moment it is *also* stored in 
   ylow in the case of true 4-column input since that is expected by 
   the sorting and drawing routines.  But this can go away later, as
   shown by your patch #4.

Disregarding patch #3 for now since it addressing a slightly different
issue,  it would be nice if you would revise patch #4 so that it 
(1) does not refer to or use the changes in patch #3
(2) gets rid of all the y value mangling, which is no longer necessary.
Instead of temporarily replacing y with VERYLARGE / UNDEFINED so that
the point count can be based on y, simply step past any points for 
with the factor (now stored in z) doesn't match.