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

Added (currently) failing test for correctness #35

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

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