Menu
▾
▴
gnuplot-beta
|
From: Daniel J S. <dan...@ie...> - 2006-06-23 07:39:22
|
To make bug 1004754 work properly and remove the grid line from outside the plot, I commented out the following lines of code from gen_tics() in axis.c:
/* {{{ a few tweaks and checks */
/* watch rounding errors */
// end += SIGNIF * step;
/* HBB 20011002: adjusting the endpoints doesn't make sense if
* some oversmart user used a ticstep (much) larger than the
* yrange itself */
// if (step < (fabs(lmax) + fabs(lmin))) {
// internal_max = lmax + step * SIGNIF;
// internal_min = lmin - step * SIGNIF;
// } else {
internal_max = lmax;
internal_min = lmin;
// }
Now I see what you were talking about, Hans. This SIGNIF is derived analogous to what I had proposed for TOL the other day. I think what I had proposed--which I'm now happy to do without--made a bit more sense than whatever this is attempting to do. Why (rhetorical question) would there be a need to tweak in any way the tic marks in such a visible way?
This should be straightforward x_i = tic * i + x_start and range check. [It also could have been done as x_i = ((last_tic - first_tic) * i) / num_tics.]
Note the use of an integer i to compute a tic location.
gen_tics() should be re-examined. I don't know if that should be done before 4.2 though.
Dan
|
|
From: Daniel J S. <dan...@ie...> - 2006-06-23 08:35:01
|
Daniel J Sebald wrote:
> To make bug 1004754 work properly and remove the grid line from outside the plot, I commented out the following lines of code from gen_tics() in axis.c:
Taking another look at this routine, let's "decode" this code. First--well let's enumerate
1) The following code
for (tic = start; tic <= end; tic += step) {
if (anyticput == 2) /* See below... */
break;
would work better as
for (tic = start; tic <= end, anyticput != 2; tic += step) {
so that the for loop exits right away once it is determined that the tics should not be plotted. That would be good, because "step" could be very small and could go through the loop many, many times.
2) I think this method of determining when to not plot the tics, i.e., setting anyticput = 2 is silly. That could be easily done BEFORE even executing the for loop. First, a more appropriate method of computing the tic should probably be to increment an integer index and compute from that, as pointed out in last email. It would be easy to compute the number of tics there are supposed to be before this foor loop. Second, look at this test:
if (anyticput) {
if (NearlyEqual(tic, start, step)) {
/* step is too small.. */
anyticput = 2; /* Don't try again. */
tic = end; /* Put end tic. */
"tic" starts out as "start". The test isn't perform the first time through. Second time through: tic = start + step, the closest that "tic" will ever be to "start". And NearlyEqual(x,y,tic) checks
fabs((tic)-(start)) < ((step) * SIGNIF))
=> fabs((start + step) - start) < (step * 0.01)
=> fabs(step) < (step * 0.01)
Which I believe is never true. Now, the only other intent here I can see is that perhaps it was meant to limit the number of tics to 100 per plot. But that can be done before the for loop and tested there. Bottom line, I think this is useless code.
This line of code looks similar:
} else if (NearlyEqual(tic, end, step)) {
but I think what this does is attempt to catch the rounding effects that might produce a "double tic" right at the end. This goes back to the fact that an integer index should be used, and in all likelihood this artifact can be avoided.
3) OK, let's say the intention of the first test in (2) was meant to limit the number of tics per plot to 100. I'm going to raise the question Why? I really don't care, but Hans made a valid point the other day about assuming some resolution of the plot and how any tiny overrun of the tic value should not be ignored. Presumably some plotting device could zoom in close enough that it could be important. Isn't the same sort of resolution restriction assumed by limiting the number of tics?
Dan
|
|
From: <br...@ph...> - 2006-06-23 22:56:51
|
Daniel J Sebald wrote:
> Daniel J Sebald wrote:
>> To make bug 1004754 work properly and remove the grid line from outside the plot, I commented out the following lines of code from gen_tics() in axis.c:
>
> Taking another look at this routine, let's "decode" this code. First--well let's enumerate
>
> 1) The following code
>
> for (tic = start; tic <= end; tic += step) {
> if (anyticput == 2) /* See below... */
> break;
>
> would work better as
>
> for (tic = start; tic <= end, anyticput != 2; tic += step) {
No, it wouldn't. The comma operator is quite wrong for a conditional
expression. It would have to be replaced by &&, which would leave you
with code exactly equivalent to the original one.
> so that the for loop exits right away once it is determined that the
> tics should not be plotted.
That's exactly what the 'break' already does.
> 2) I think this method of determining when to not plot the tics,
> i.e., setting anyticput = 2 is silly.
It's not. It's complicated because it has to be. We learned that the
hard way, years ago. The code looks the way it does because it was the
simplest approach we found that actually worked reliably, even in
extreme cases.
> That could be easily done
> BEFORE even executing the for loop.
No, it can't. Numerical maths on an optimizing compiler is trickier
than you know. Things that look they should be mathematically
equivalent, rather often aren't.
> First, a more appropriate method
> of computing the tic should probably be to increment an integer index
> and compute from that, as pointed out in last email. It would be
> easy to compute the number of tics there are supposed to be before
> this foor loop.
It would be easy to compute, sure. Unfortunately, it can just as easily
be way off. Especially where floating point maths close to its limit of
precision is involved:
> => fabs((start + step) - start) < (step * 0.01)
> => fabs(step) < (step * 0.01)
That last line does *not* follow from the one above. Not where
floating-point arithmetic reigns.
|
|
From: Daniel J S. <dan...@ie...> - 2006-06-23 23:12:29
|
I just about have a patch together for this. I don't think this SIGNIF a=
nd stuff is necessary. I think this is what you were referring to before=
about resolution sorts of this propogating about.
The idea here is to overlay a series of tics, major AND minor, to cover t=
he viewable range, or the range specified by the user that is within the =
viewable range. Because we want to include minor tics, that means we sho=
uld push the minor tics out one further than what is visible on the plot.
>From there, generate the tic (major or minor); if it is in the visible ra=
nge plot it otherwise don't.
There are more elegant ways of dealing with precision problems. For exam=
ple, rather than doing this:
if (start > end) {
/* put in order */
double numtics =3D floor((end * (1 + SIGNIF) - start) / incr);
end =3D start;
start =3D end + numtics * incr;
incr =3D -incr;
}
how about
if (start > end) {
/* put in order */
double numincr =3D floor((end - start) / incr);
/* Deal with rounding issues this way */
if (start + (numincr + 1) * incr >=3D end)
numincr++;
end =3D start;
start =3D end + numincr * incr;
incr =3D -incr;
}
Hans-Bernhard Br=F6ker wrote:
> It would be easy to compute, sure. Unfortunately, it can just as easil=
y=20
> be way off. Especially where floating point maths close to its limit o=
f=20
> precision is involved:
>=20
>> =3D> fabs((start + step) - start) < (step * 0.01)
>> =3D> fabs(step) < (step * 0.01)
>=20
>=20
> That last line does *not* follow from the one above. Not where=20
> floating-point arithmetic reigns.
Well, sure the case where start + step =3D start because of loss of resol=
ution. But is that a situation where there will be a meaningful plot? I=
t seems a convoluted way of deal with resolution problems.
I think we can get away from having to use such things. I'll upload the =
patch this evening some time.
Dan
|
|
From: Petr M. <mi...@ph...> - 2006-06-28 15:07:58
|
> I've placed a patch under bug report [ 1004754 ] on SourceForge that is a > cleanup of the tic generation. I think it would be worth considering for > 4.2, because it does fix the bug and it is much friendlier computer math I propose to commit it. Notes: "logorithm" => "logarithm" (I wish also the "rounding" plot [-1:1] patch...) --- PM |
|
From: Daniel J S. <dan...@ie...> - 2006-06-28 16:48:04
|
Petr Mikulik wrote: >> I've placed a patch under bug report [ 1004754 ] on SourceForge that >> is a cleanup of the tic generation. I think it would be worth >> considering for 4.2, because it does fix the bug and it is much >> friendlier computer math > > > I propose to commit it. > > Notes: "logorithm" => "logarithm" > > (I wish also the "rounding" plot [-1:1] patch...) It's up to Ethan and Hans, depending on how things are honing in on 4.2. It sounds like Ethan wants to only get the most obvious of bugs with a few lines diff at this point. I would like to see it considered right after 4.2 though. In general, looking through the bug list, I can see that time data and tics is going to be an issue. With the way time data currently works (setting the minitic to a certain value rather than a fraction of the major tic) suggests to me that minitics will have to be the basis in that case, not the other way around. So, before revisiting time data tics, this patch should be considered. I'll look for the misspelling "logorithm". Dan |
|
From: Daniel J S. <dan...@ie...> - 2006-06-25 23:23:17
|
I've placed a patch under bug report [ 1004754 ] on SourceForge that is a cleanup of the tic generation. I think it would be worth considering for 4.2, because it does fix the bug and it is much friendlier computer math and programming wise in my opinion--very clean. I think I've covered everything, such that there is a good chance any outstanding tic placement bugs should have been addressed by this. Please take a look at the patch and see what you think. I've put detailed comments at the key points. It should be clear in the patch why things are done the way they are. There really isn't too much changed. Now is the best time to work on this, since it is fresh and I will soon get busy on other projects. The reason I think what this patch is an improvement is that it handles rounding artifacts in several different places where they crop up rather than trying to solve them all with a single conditional test right before the tic-placement in the for-loop. This patch does not have any type of SIGNIF and tolerance based upon the range. It only deals with rounding effects at the level of DBL_EPSILON or order thereof. Furthermore, the tic placement is done more in an integerized fashion, i.e., an integer start and integer end corresponding to the start and end values after factoring out the step increment are computed. We then know before entering the loop what the number of intervals, N_int, should be. (An interval is the major tic with the group of potential minor tics to its positive side.) Being more accurate with respect to the integer equivalents, the code address any roundoff problem with the major tic end point as: /* Address rounding issues in the following way */ if (i_tic == (N_int - 1)) tic = end; else tic = start + i_tic * step; to avoid losing a tic at the end range. I've checked every example in 'all.dem' for consistency before and after the patch and that the patch fixes the bug of grid lines sometimes landing outside the borders. In fact, 'all.dem' helped a great deal for debugging. Dan |
|
From: Ethan M. <merritt@u.washington.edu> - 2006-06-28 16:26:35
|
On Wednesday 28 June 2006 08:07 am, Petr Mikulik wrote: > > I've placed a patch under bug report [ 1004754 ] on SourceForge > > that is a cleanup of the tic generation. I think it would be worth > > considering for 4.2, because it does fix the bug and it is much > > friendlier computer math > > I propose to commit it. > > Notes: "logorithm" => "logarithm" I have not analyzed the code itself, and don't have time to do so right now. But the cover explanation contains an off-by-one error in the algorithm, so I worry that it is not correct. > number of intervals, N_int > /* Address rounding issues in the following way */ > if (i_tic == (N_int - 1)) > tic = end; > else > tic = start + i_tic * step; This is an off-by-one error, an instance of the "fencepost problem". The obvious case is when there is one interval, so (N_int-1) == 0 , and the above code puts both tics at the same end of the axis. > > (I wish also the "rounding" plot [-1:1] patch...) > > --- > PM > > > Using Tomcat but need to do more? Need to support web services, > security? Get stuff done quickly with pre-integrated technology to > make your job easier Download IBM WebSphere Application Server > v.1.0.1 based on Apache Geronimo > http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121 >642 _______________________________________________ > gnuplot-beta mailing list > gnu...@li... > https://lists.sourceforge.net/lists/listinfo/gnuplot-beta -- Ethan A Merritt Biomolecular Structure Center University of Washington, Seattle WA |
|
From: Daniel J S. <dan...@ie...> - 2006-06-28 17:03:30
|
Ethan Merritt wrote:
> On Wednesday 28 June 2006 08:07 am, Petr Mikulik wrote:
>
>>>I've placed a patch under bug report [ 1004754 ] on SourceForge
>>>that is a cleanup of the tic generation. I think it would be worth
>>>considering for 4.2, because it does fix the bug and it is much
>>>friendlier computer math
>>
>>I propose to commit it.
>>
>>Notes: "logorithm" => "logarithm"
>
>
> I have not analyzed the code itself, and don't have time to
> do so right now. But the cover explanation contains an
> off-by-one error in the algorithm, so I worry that it is not
> correct.
That is fine in this new layout. It's explained in the note. (But if you are uncomfortable with it, leave it out for now.)
I will use [ ] to signify range (i.e., lmin and lmax), and I will use T to be major tic and t to be minor tic.
[ ]
t T t t t t T t
^ ^
start outside
i_tic= 0 0 0 0 0 1 1 ...
The scheme is to first "integerize" the major tics where "start" will be the case of i_tic = 0. (I should say, that we don't round the upper limit upward anymore, but downward because if it is outside of the range, it isn't printed.) In this case N_start and N_end are the same. We only need one interval and minor tics.
Imagine the case of moving that upper range "]" so that it equals a major tic. Well in that case, with the rounding downward (floor) it will be that N_end = N_start + 1, and we'll get two intervals of major tic and minor tics. (The minor tics will be outside the range and not printed, but that is the idea.)
Dan
|
