Skip to content

Added (currently) failing test for correctness#35

Draft
k-dominik wants to merge 1 commit intomainfrom
error_test
Draft

Added (currently) failing test for correctness#35
k-dominik wants to merge 1 commit intomainfrom
error_test

Conversation

@k-dominik
Copy link
Contributor

imo marching cubes should deliver correct results under the assumption of C-ordered arrays with x as fastest varying index.
I added a test that demonstrates that this is currently not the case; sample test output:

=================================== FAILURES ===================================
__________________________ test_marching_orientation ___________________________

data = array([[[0, 0, 0, ..., 0, 0, 0],
        [0, 0, 0, ..., 0, 0, 0],
        [0, 0, 0, ..., 0, 0, 0],
        ...,
      ......,
        [0, 0, 0, ..., 0, 0, 0],
        [0, 0, 0, ..., 0, 0, 0],
        [0, 0, 0, ..., 0, 0, 0]]], dtype=uint32)

    def test_marching_orientation(data):
        v, _, _ = marching_cubes.march(data, 0)
        max_v = v.max(axis=0)
>       numpy.testing.assert_array_almost_equal(max_v, [10.5, 8.5, 6.5])
E       AssertionError: 
E       Arrays are not almost equal to 6 decimals
E       
E       Mismatched elements: 2 / 3 (66.7%)
E       Max absolute difference: 4.
E       Max relative difference: 0.61538462
E        x: array([ 6.5,  8.5, 10.5], dtype=float32)
E        y: array([10.5,  8.5,  6.5])

here one can see, that z and x coordinates are flipped.

See also #34

@k-dominik k-dominik marked this pull request as draft September 30, 2020 14:04
@DerThorsten
Copy link
Contributor

DerThorsten commented Sep 30, 2020

I suggest the following:
instead of these shitty calls:

volume[z*xyWidth + y*xWidth + x] < isoLevel

we use xtensor / pyxtensor
and use sane calls like

xtensor_volume(x,y,z) < isoLevel

Since the offsets are anyhow fresh computed for any call into volume[...]. So there is zero benefit in doing it like this.
One might have to change the loop order here:

for (size_t z = 0; z < zDim; z++)

but besides from that, it should be easy. And these stride related issues should be gone (and the impl is finally in C-order not fortran order)

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

Successfully merging this pull request may close these issues.

2 participants