Bug in DateAxis?

[hookumsnivy]

David, please correct me if I am wrong:

The method previousStandardDate is supposed to do the following:
Given a date and a tick unit, return a date that is unit.getCount() units (month, day, hour, etc) before the date. It should also take into account the tick mark position.

So for instance if you are given March 31, 2004 23:59:59.999 and your unit is a Month with a count of 3, and a tick mark position of END the method should return: December 31, 2003 23:59:59.999

If that is the case, the code is wrong since it will incorrectly return January 31, 2004 23:59:59.999 which is only two months before the passed in date.

The logic isn't taking into account the special cases of a calendar. Instead of using arithmetic, the calendar methods should be used.

I modified the method to be the following and I think it is much easier to understand:

Code: Select all

    protected Date previousStandardDate(Date date, DateTickUnit unit) {

        Calendar calendar = Calendar.getInstance(this.timeZone);
        calendar.setTime(date);
        int count = unit.getCount();

        switch (unit.getUnit()) {

            case (DateTickUnit.MILLISECOND) :
                calendar.add(Calendar.MILLISECOND, -count);
                return calendar.getTime();
            case (DateTickUnit.SECOND) :
                calendar.add(Calendar.SECOND, -count);
                return calculateDateForPosition(new Second(calendar.getTime()), this.tickMarkPosition);

            case (DateTickUnit.MINUTE) :
                calendar.add(Calendar.MINUTE, -count);
                return calculateDateForPosition(new Minute(calendar.getTime()), this.tickMarkPosition);

            case (DateTickUnit.HOUR) :
                calendar.add(Calendar.HOUR, -count);
                return calculateDateForPosition(new Hour(calendar.getTime()), this.tickMarkPosition);

            case (DateTickUnit.DAY) :
                calendar.add(Calendar.DATE, -count);
                return calculateDateForPosition(new Day(calendar.getTime()), this.tickMarkPosition);

            case (DateTickUnit.MONTH) :
                calendar.add(Calendar.MONTH, -count);
                return calculateDateForPosition(new Month(calendar.getTime()), this.tickMarkPosition);

            case(DateTickUnit.YEAR) :
                calendar.add(Calendar.YEAR, -count);
                return calculateDateForPosition(new Year(calendar.getTime()), this.tickMarkPosition);

            default: return null;

        }

    }

It will also take care of tracker #1510805 (the code someone posted there will not work in all cases).

I didn't know how to contribute the code, so I just put it here.

If I'm correct, let me know and I'll respond to the tracker ticket.

[david.gilbert]

David, please correct me if I am wrong:

The method previousStandardDate is supposed to do the following:
Given a date and a tick unit, return a date that is unit.getCount() units (month, day, hour, etc) before the date. It should also take into account the tick mark position.

More or less right. The idea is that you can take an arbitrary date (usually the lower bound of the DateAxis, which can be *any* date) and then look at a sequence of tick mark dates (which typically occur at regular intervals and fall on nice "rounded" date/time values) and choose the latest tick mark date that is *prior* to the arbitrary reference date.

The problem is that I've underspecified the DateTickUnit class for the case where you have multiples of some Calendar unit. If tick marks should be displayed every 3 months, for example, we don't know if that means (JAN, APR, JUL, OCT) or (FEB, MAY, AUG, NOV) or (MAR, JUN, SEP, DEC). There is a similar problem for days of the week.

At present, the previousStandardDate() method just ignores the sliding effect of the Calendar unit multiples, and takes the most recent single unit, then calculates the multiples from there.

Your code (which I like the look of, but need to test some more to make sure it really does work as it should) will, I think, slide the tick mark dates a little differently (the first visible tick mark on the axis will appear earlier in some cases, which wouldn't be a bad thing).

Ideally, some way for the user to specify a preferred month or day (or other unit) as the base for multiple unit calculations would be ideal.

[david.gilbert]

I tried your code and it isn't correct, although I'm going to have a go at fixing it because I think you are on to a cleaner implementation than what is currently there.

One of the key properties of the previousStandardDate() method for a given date/time t is that if d0 is the value from previousStandardDate() and d1 is d0 + the tick unit, then:

d0 < t <= d1

In other words, d0 will not be visible on the axis, but d1 will (assuming that t is the lower bound of the date axis, which it usually is when the previousStandardDate() method is called.

I committed a whole bunch of JUnit tests to CVS to check this property, and fixed a couple of minor bugs in the current implementation. When I switch to your implementation, I get a lot of failures. Also, visually the time series charts in the demo look broken. Now I'm going to go and see if there is a clean fix to make your code pass the tests...

[david.gilbert]

Here's a revision of your code that *almost* passes all the tests:

Code: Select all

    protected Date previousStandardDate(Date date, DateTickUnit unit) {

        Calendar calendar = Calendar.getInstance(this.timeZone);
        calendar.setTime(date);
        int count = unit.getCount();

        switch (unit.getUnit()) {

            case (DateTickUnit.MILLISECOND) :
                calendar.add(Calendar.MILLISECOND, -count);
                return calendar.getTime();
            case (DateTickUnit.SECOND) :
                Second s = new Second(date, this.timeZone);
                Date sd = calculateDateForPosition(s, this.tickMarkPosition);
                if (sd.getTime() >= date.getTime()) {
                    calendar.add(Calendar.SECOND, -count);
                    s = new Second(calendar.getTime(), this.timeZone);
                    sd = calculateDateForPosition(s, this.tickMarkPosition);
                }
                return sd;
                
            case (DateTickUnit.MINUTE) :
                Minute m = new Minute(date, this.timeZone);
                Date md = calculateDateForPosition(m, this.tickMarkPosition);
                if (md.getTime() >= date.getTime()) {
                    calendar.add(Calendar.MINUTE, -count);
                    m = new Minute(calendar.getTime(), this.timeZone);
                    md = calculateDateForPosition(m, this.tickMarkPosition);
                }
                return md;

            case (DateTickUnit.HOUR) :
                Hour h = new Hour(date, this.timeZone);
                Date hd = calculateDateForPosition(h, this.tickMarkPosition);
                if (hd.getTime() >= date.getTime()) {
                    calendar.add(Calendar.HOUR, -count);
                    h = new Hour(calendar.getTime(), this.timeZone);
                    hd = calculateDateForPosition(h, this.tickMarkPosition);
                }
                return hd;

            case (DateTickUnit.DAY) :
                Day d = new Day(date, this.timeZone);
                Date dd = calculateDateForPosition(d, this.tickMarkPosition);
                if (dd.getTime() >= date.getTime()) {
                    calendar.add(Calendar.DATE, -count);
                    d = new Day(calendar.getTime(), this.timeZone);
                    dd = calculateDateForPosition(d, this.tickMarkPosition);
                }
                return dd;

            case (DateTickUnit.MONTH) :
                Month mm = new Month(date, this.timeZone);
                Date mmd = calculateDateForPosition(mm, this.tickMarkPosition);
                if (mmd.getTime() >= date.getTime()) {
                    calendar.add(Calendar.MONTH, -count);
                    mm = new Month(calendar.getTime(), this.timeZone);
                    mmd = calculateDateForPosition(mm, this.tickMarkPosition);
                }
                return mmd;

            case(DateTickUnit.YEAR) :
                Year y = new Year(date, this.timeZone);
                Date yd = calculateDateForPosition(y, this.tickMarkPosition);
                if (yd.getTime() >= date.getTime()) {
                    calendar.add(Calendar.YEAR, -count);
                    y = new Year(calendar.getTime(), this.timeZone);
                    yd = calculateDateForPosition(y, this.tickMarkPosition);
                }
                return yd;

            default: return null;

        }

    } 

There's still a problem caused by the calculation for the middle millisecond in the RegularTimePeriod classes - you'll see one of the JUnit tests for the Year interval failing. I think this is because the "middle" millisecond for one year isn't necessarily exactly n years from the "middle" millisecond for another year, when using Calendar date arithmetic. For example, when one of the years is a leap year.

I'll think I'll leave the previousStandardDate() method as-is, for now...although it's been a good diversion in the sense that a few boundary case bugs have now been fixed!

[hookumsnivy]

Thanks for taking a look at it.

I obviously missed some test cases. I didn't take into account that the date passed in would be at the end of the month while the tick mark position would be at the beginning. In my code, the resulting date would actually be 2 dates prior to the passed in date in the case described above.

I'm going to take a look at the unit tests to see what other tests are failing, but it makes sense that the middle millisecond for a leap year isn't exactly n years away from another year. It can be fixed by always using a specific day of the year for the middle millisecond of a year, but then you don't have the exact middle for leap years.

I'm not sure there's a perfect solution to this issue if you want the exact middle.

[david.gilbert]

I'm not sure there's a perfect solution to this issue if you want the exact middle.

By passing the getMiddleMillisecond() method call in calculateDateForPosition() might be a way to resolve this.

[airon]

This problem seems to be related to the one than I describe in viewtopic.php?t=20993. Mine is quite similar, in the same method, but it involves the "minute". I suggest there a little fix that works for me.