Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tests give false positives, as _assertions requires a1.all() == a2.all() instead of np.all(a1==a2) #202

Open
JamesParrott opened this issue Dec 14, 2023 · 0 comments

Comments

@JamesParrott
Copy link

JamesParrott commented Dec 14, 2023

It's really great there's a working test framework and so many tests.

However, in mapclassify\tests\test_classify.py, the only thing being tested really, is that the big if elif elif ... block in mapclassify\_classify_API.py:classify returns the correct class, and creates an instance with the correct args

That would be fine, it might help pick up mistakes early. But unfortunately there was some confusion about how numpy.all works and should be used in tests - see below. An array.all() simply returns a scalar, True if all its elements are non-zero (Truthy), e.g.:

assert numpy.asarray([random.randint(1,100) for __ in range(8)]).all()

So apart from k being equal (the number of classes) mapclassify\tests\test_classify.py:_assertions tests nothing but that all three array variables of a and b, yb, bins and counts, are all nonzero (or both contain a zero):

def _assertions(a, b):
    assert a.k == b.k
    assert a.yb.all() == b.yb.all()
    assert a.bins.all() == b.bins.all()
    assert a.counts.all() == b.counts.all()

Other than that, those arrays could contain anything at all (as long as they're all non-zero, or both contain a zero), and still pass _assertions.

FisherJenksSample.__init__ and other classes extract random samples from the provided data, so are stochastic by their very nature.

The result of one call to FisherJenksSample should not equal another call to it, by definition (the same for any of the other classes that take random samples or are otherwise stochastic).

A correct form of mapclassify\tests\test_classify.py:_assertions looks like:

def _assertions(a, b):
    assert a.k == b.k
    assert (a.yb == b.yb).all()
    assert (a.bins == b.bins).all()
    assert (a.counts == b.counts).all()

If this means that something fails, then great, that's something to fix. But tests shouldn't be written to mask bugs or allow two arrays of random numbers to supposedly equal each other, just because neither contains a zero, or both do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant