Chronological Thread 

< Chronological >      < Thread >

---

I looked at revision 2065, and I agree, that is far closer to the style I
think we should follow. Can we get rid of the CHECK_DUP macro? Personally I
don't like macros.

Just a word about the process here. I think we should always be open to any
code that we submit being reworked like this. Very often, one is initially
focused on the algorithm, and it takes a second pass to improve the style.
Obviously with time the style will become habit, and be part of the first
pass.

In other words, good work to both of you, Justin and Andrew.

Regards,
Randall

> -----Original Message-----
> From: cellml-tools-developers-bounces at cellml.org [mailto:cellml-tools-
> developers-bounces at cellml.org] On Behalf Of Andrew Miller
> Sent: Thursday, 17 January 2008 11:24 a.m.
> To: cellml-tools-developers at cellml.org
> Subject: Re: [cellml-dev] r2062 - pce/trunk/DataCollector
>
> CellML Automated Notifications wrote:
> > Author: jmar150
> > Date: 2008-01-17 01:11:18 +1300 (Thu, 17 Jan 2008)
> > New Revision: 2062
> >
> > Modified:
> > pce/trunk/DataCollector/DataCollector.cpp
> > Log:
> > Progress towards Tracker item 23, now that PCEnv 0.3 has been
> released.
> > I attempted to keep the display of more of the extreme features of a
> graph zoom invariant.
> > A naive approach was used, simply caching the maxima and minima in
> both axes of entire skipped regions.
> >
> Hi,
>
> Just a few review comments regarding style that came up (I have
> committed a change addressing these). I hope everyone likes the new
> version better - otherwise I can revert it.
>
> 1) I found the xb / yb / qb naming convention somewhat confusing when
> reading through the code. I changed this to be a structure, so e.g.
>
> xb[0] becomes ExtremePoints[MIN_X].X
> while qb[3] becomes ExtremePoints[MAX_Y].index
> I find this slightly easier to follow because you don't need to know
> what the indices mean.
>
> 2) I don't like the idea of writing sorting code in place (although
> there are probably examples elsewhere in the code where I may have done
> the same thing...). Ideally, we could use std::sort and boost lamda
> library and do the sort without any out-of-line code, but that is not
> really encouraged in XPCOM code and we don't have a dependency on boost
> at this stage. However, XPCOM glue provides NS_QuickSort (yes, I know
> the overhead of calling it probably costs slightly more than using
> bubblesort for such a small set, but I think that it might be inlined
> anyway, and code readability is important too). I think we could review
> this if it becomes a performance issue, although with the skip code in
> (and this only running once for each skipped set of data) it probably
> won't.
>
> Best regards,
> Andrew
>
> > Modified: pce/trunk/DataCollector/DataCollector.cpp
> > ===================================================================
> > --- pce/trunk/DataCollector/DataCollector.cpp 2008-01-16 02:39:09
UTC
> (rev 2061)
> > +++ pce/trunk/DataCollector/DataCollector.cpp 2008-01-16 12:11:18
UTC
> (rev 2062)
> > @@ -467,6 +467,7 @@
> > NS_ENSURE_TRUE(mRenderContext,);
> > NS_ENSURE_TRUE((mxVariable < nvar) && (myVariable < nvar),);
> >
> > +
> > while (PR_TRUE)
> > {
> > mRenderContext->BeginPath();
> > @@ -481,6 +482,11 @@
> > PRBool countOnlyMode = PR_FALSE;
> > PRUint32 start0 = start;
> >
> > + PRFloat64 xb[4];
> > + PRFloat64 yb[4];
> > + PRUint32 tb[4];
> > + PRUint32 qb[4];
> > +
> > if (mGlyph == IDataToCanvasRequest::GLYPH_LINES)
> > {
> > // We need to include the previous point if available...
> > @@ -587,11 +593,75 @@
> > }
> >
> > pointsSoFar++;
> > - if (countOnlyMode)
> > - continue;
> > if (pointsSoFar > 2048 && skip == 1)
> > {
> > countOnlyMode = PR_TRUE;
> > + xb[0] = xb[1] = xb[2] = xb[3] = xI[1];
> > + yb[0] = yb[1] = yb[2] = yb[3] = yI[1];
> > + qb[0] = qb[1] = qb[2] = qb[3] = i;
> > + tb[0] = 0;
> > + tb[1] = 1;
> > + tb[2] = 2;
> > + tb[3] = 3;
> > + }
> > + if (countOnlyMode)
> > + {
> > + if(xI[1] < xb[0])
> > + {
> > + xb[0] = xI[1];
> > + yb[0] = yI[1];
> > + qb[0] = i;
> > + }
> > + if(xI[1] > xb[1])
> > + {
> > + xb[1] = xI[1];
> > + yb[1] = yI[1];
> > + qb[1] = i;
> > + }
> > + if(yI[1] < yb[2])
> > + {
> > + xb[2] = xI[1];
> > + yb[2] = yI[1];
> > + qb[2] = i;
> > + }
> > + if(yI[1] > yb[3])
> > + {
> > + xb[3] = xI[1];
> > + yb[3] = yI[1];
> > + qb[3] = i;
> > + }
> > + if ((i + skip) >= (end - skip))
> > + {
> > + // Now we render the maximal and minimal points among
> the set skipped
> > + // First, order by bvar index using a horrible
> bubblesort
> > + PRUint32 j, k, temp;
> > + for(j = 0; j < 4; j++)
> > + {
> > + for(k = j; k < 4; k++)
> > + {
> > + if(qb[tb[j]] > qb[tb[k]])
> > + {
> > + temp = tb[k];
> > + tb[k] = tb[j];
> > + tb[j] = temp;
> > + }
> > + }
> > + }
> > +#define CHECK_DUP(a, b, c) \
> > +if (!((xb[tb[a]]==xb[tb[b]])&&(yb[tb[a]]==yb[tb[b]]))) \
> > + c;
> > +
> > + mRenderContext->LineTo(mxMultiplier * xb[tb[0]] +
> mxOffset,
> > + myMultiplier * yb[tb[0]] +
> myOffset);
> > + CHECK_DUP(0, 1, mRenderContext->LineTo(mxMultiplier *
> xb[tb[1]] + mxOffset,
> > + myMultiplier * yb[tb[1]] +
> myOffset));
> > + CHECK_DUP(1, 2, mRenderContext->LineTo(mxMultiplier *
> xb[tb[2]] + mxOffset,
> > + myMultiplier * yb[tb[2]] +
> myOffset));
> > + CHECK_DUP(2, 3, mRenderContext->LineTo(mxMultiplier *
> xb[tb[3]] + mxOffset,
> > + myMultiplier * yb[tb[3]] +
> myOffset));
> > + mRenderContext->Stroke();
> > +#undef CHECK_DUP
> > + }
> > continue;
> > }
> >
> > @@ -621,11 +691,65 @@
> > continue;
> >
> > pointsSoFar++;
> > - if (countOnlyMode)
> > - continue;
> > if (pointsSoFar > 2048 && skip == 1)
> > {
> > countOnlyMode = PR_TRUE;
> > + xb[0] = xb[1] = xb[2] = xb[3] = x;
> > + yb[0] = yb[1] = yb[2] = yb[3] = y;
> > + }
> > + if (countOnlyMode)
> > + {
> > + if(x < xb[0])
> > + {
> > + xb[0] = x;
> > + yb[0] = y;
> > + }
> > + if(x > xb[1])
> > + {
> > + xb[1] = x;
> > + yb[1] = y;
> > + }
> > + if(y < yb[2])
> > + {
> > + xb[2] = x;
> > + yb[2] = y;
> > + }
> > + if(y > yb[3])
> > + {
> > + xb[3] = x;
> > + yb[3] = y;
> > + }
> > + if ((i + skip) <= end)
> > + {
> > + for (int j = 0; j <= 3; j++)
> > + {
> > + x = mxMultiplier * xb[j] + mxOffset;
> > + y = myMultiplier * yb[j] + myOffset;
> > + switch (mGlyph)
> > + {
> > + case IDataToCanvasRequest::GLYPH_SQUARES:
> > + mRenderContext->StrokeRect(x - 5.0, y - 5.0, 9.0,
> 9.0);
> > + break;
> > + case IDataToCanvasRequest::GLYPH_CIRCLES:
> > + mRenderContext->BeginPath();
> > + mRenderContext->MoveTo(x + 4.0, y);
> > + mRenderContext->Arc(x, y, 4.0, 0.0, 6.28318530717959,
> PR_TRUE);
> > + mRenderContext->Stroke();
> > + break;
> > + case IDataToCanvasRequest::GLYPH_DIAMONDS:
> > + mRenderContext->Save();
> > + // Rotate by 45deg (1/4 pi radians)...
> > + mRenderContext->Translate(x, y);
> > + mRenderContext->Rotate(0.785398163397448);
> > + mRenderContext->StrokeRect(-5.0, -5.0, 9.0, 9.0);
> > + mRenderContext->Restore();
> > + break;
> > + case IDataToCanvasRequest::GLYPH_DOTS:
> > + mRenderContext->FillRect(x - 1.0, y - 1.0, 3.0, 3.0);
> > + break;
> > + }
> > + }
> > + }
> > continue;
> > }
> >
> >
> > _______________________________________________
> > automated-notifications mailing list
> > automated-notifications at cellml.org
> > http://www.cellml.org/mailman/listinfo/automated-notifications
> >
>
> _______________________________________________
> cellml-tools-developers mailing list
> cellml-tools-developers at cellml.org
> http://www.cellml.org/mailman/listinfo/cellml-tools-developers

---

• [cellml-dev] r2062 - pce/trunk/DataCollector, Andrew Miller, 01/17/2008
  • [cellml-dev] r2062 - pce/trunk/DataCollector, Randall Britten, 01/17/2008
    • [cellml-dev] r2062 - pce/trunk/DataCollector, Andrew Miller, 01/17/2008

---

Archive powered by MHonArc 2.6.18.