newConeComputerApprox failure

[mbtaylor]

@fxpineau, I'm getting an ArrayIndexOutOfBoundsException for this code:

import cds.healpix.Healpix;
import cds.healpix.HealpixNestedFixedRadiusConeComputer;

public class T1 {
    public static void main(String[] args) {
        int depth = 14;
        double radius = 0.001;
        double alpha = 0.002;
        double delta = -1.3;
        HealpixNestedFixedRadiusConeComputer coner =
            Healpix.getNested(depth).newConeComputerApprox(radius);
        coner.overlappingCells(alpha, delta);      // ArrayOutOfBoundsException here
    }
}

It does not seem too dependent on the numerical values. Can you take a look?

[fxpineau]

Thank you for having reported the issue. It is the same as issue #12. I have added a test and replaced the previous MOC upper bound size estimation by a (supposedly) more robust estimation.

[mbtaylor]

In #12, you wrote "I promise that if you encounter again a problem, I will make the effort to implement a grow-able object." Is the promise renewed :-) ?

[fxpineau]

Yes, you are right :)
For my defense (for what it is worth): instead of just increasing the factor 4-->6(-->8) in nMocCellInConeUpperBound, I changed the estimate. By-the-way, if someone find a better one (I am sure there are better one's), I will take it.
I give it another (last?) chance before introducing a test on the array size at each cell insertion. (If you really prefer a growable array, I may create a new branch: would you agree to perform tests with e.g. TOPCAT cross-matches to check whether the additional test performance penalty is negligible or not (I guess it is, but ...)?)

[mbtaylor]

I would like to have a reliable implementation here; these issues are from user reports of (to users) incomprehensible match failures, and (a) fixing it means a library upgrade in topcat/stilts which doesn't make it out to users until the next public release is made and installed, plus (b) for each such error that's reported there may be several where people just give up. So, I'd like to avoid another possible increase-hardwired-bound cycle in the future if possible.

How about this for a growable list implementation:

/**
 * Variable length list of longs.
 * The capacity supplied at construction time should be big enough to
 * contain all the elements, since exceeding it will be relatively expensive
 * (it involves throwing and catching an exception).
 * But it won't be catastrophic.
 */
public class LongList {

    public long[] array;
    public int count;

    /**
     * @param  capacity  expected upper bound for capacity of list
     */
    public LongList(int capacity) {
        array = new long[capacity];
    }

    public void append(long value) {
        try {
            array[count] = value;
        }
        catch ( ArrayIndexOutOfBoundsException e ) {
            long[] array1 = new long[2 * array.length];
            System.arraycopy(array, 0, array1, 0, array.length);
            array = array1;
            array[count] = value;
        }
        count++;
    }
}

It's not a good general-purpose implementation because of the expense of increasing the capacity, but if you're confident that the specified capacity is high enough it's hard to imagine it being detectably more expensive than a fixed array, and in the event that you're wrong about the supplied capacity it will still work. It would be easy to adapt for less expensive capacity increase, just requires an extra test during the append (the primitive array implementation is presumably doing this in any case). Of course, no problem to test it in stilts if you agree.

Plus some test code as an added extra:

    private static void test(int capacity, int n) {
        LongList list = new LongList(capacity);
        for (int i = 0; i < n; i++) {
            list.append(i);
        }
        long[] array = list.array;
        int count = list.count;

        boolean ok = true;
        ok = ok && (count == n);
        for (int i = 0; i < n; i++) {
            ok = ok && ( array[i] == i );
        }
        if (!ok) {
            throw new RuntimeException("Failure");
        }
    }

    public static void main( String[] args ) {
        for (int ic = 10; ic < 100; ic++) {
            for (int in = 0; in < 100; in++) {
                test(ic, in);
            }
        }
    }

[fxpineau]

I understand, and in the meantime I also decided to (quickly) implemented a growable array. I see in your code that your are not using an extra test but the Java built-in bound check (using try/catch): I wish I would have thought of it. I take the idea, thank you! I so far print an ugly message to be aware of array size increase (that I would like to prevent). Tell me if you want me to remove it / have a better idea.

[mbtaylor]

OK great, thanks. To report the unexpected/unwanted array size increase I'd probably write a WARNING message through the java.util.logging system, but an ugly print message is fine, since you're not expecting it to happen (it's better than an uncaught exception).

[fxpineau]

I have added your tests and replaced sysout by java.util.logging like you suggested (thank you).