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

ENH: allow ndimage to interpolate intp arrays #64

Merged
merged 3 commits into from
Aug 29, 2011

Conversation

matthew-brett
Copy link
Contributor

Add tests of use of input datatypes in ndimage interpolation C routines.

Add tests of use of input datatypes in ndimage interpolation C routines.
The maxes were set to MAX_UINT32 and labeled 'fixme'; this was probably
because of an error in the numpy includes for MAX_UINT64, leading to a
compiler error.  We just work round it by using a correctly defined
version.
@stefanv
Copy link
Member

stefanv commented Aug 28, 2011

The test given in the first patch does not fail for me on the current version of ndimage. Int64 and intp should be the same data types, so I don't think we need to handle them separately?

The second patch looks good, but let's add a test to verify its behaviour.

@stefanv
Copy link
Member

stefanv commented Aug 28, 2011

Here's a test for your second patch:

https://github.com/stefanv/scipy/tree/intp_fix_update

@matthew-brett
Copy link
Contributor Author

On Sun, Aug 28, 2011 at 2:28 AM, stefanv
reply@reply.github.com
wrote:

The test given in the first patch does not fail for me on the current version of ndimage.  Int64 and intp should be the same data types, so I don't think we need to handle them separately?

It won't fail on 64 bits I guess. I was working on 32 bits.

32 bits OSX:

In [2]: np.dtype(np.intp).num
Out[2]: 5

In [3]: np.dtype(np.int32).num
Out[3]: 7

In [7]: np.dtype(np.int64).num
Out[7]: 9

64 bits Linux:

In [2]: np.dtype(np.intp).num
Out[2]: 7

In [3]: np.dtype(np.int32).num
Out[3]: 5

In [4]: np.dtype(np.int64).num
Out[4]: 7

So it looks like, on 64 bits, the intp type is the same by number as
the int64, but not so for 32 bits, at least on one system. I guess
you are working on 64 bits. In any case, of course it doesn't matter
that we test for the same type twice, because all the macros have
'breaks' so that the data won't be processed twice.

Thanks to Stefan van der Walt for the test idea.
@matthew-brett
Copy link
Contributor Author

The test given in the first patch does not fail for me on the current version of ndimage.  Int64 and intp should be the same data types, so I don't think we need to handle them separately?

It won't fail on 64 bits I guess.  I was working on 32 bits.

Here's the test failure for scipy 0.9.0 on OSX 32 bits:

test_datatypes.test_map_coordinates_dts ... ERROR

ERROR: test_datatypes.test_map_coordinates_dts

Traceback (most recent call last):
File "/Users/mb312/usr/local/lib/python2.6/site-packages/nose-1.1.3.dev-py2.6.egg/nose/case.py",
line 197, in runTest
self.test(*self.arg)
File "/Users/mb312/dev_trees/scipy/scipy/ndimage/tests/test_datatypes.py",
line 37, in test_map_coordinates_dts
out = ndimage.map_coordinates(these_data, coords_m1, order=order)
File "/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/site-packages/scipy/ndimage/interpolation.py",
line 298, in map_coordinates
output, order, mode, cval, None, None)
RuntimeError: coordinate array data type not supported

@stefanv stefanv merged commit 2bbcb3f into scipy:master Aug 29, 2011
@pv
Copy link
Member

pv commented Aug 29, 2011

This change breaks compilation on 64-bit, where NPY_INT64 == NPY_INTP which causes duplicate case labels. I tried to fix it in 164c7c8 -- please test again.

The different Numpy numeric types are defined in the enum NPY_TYPES in ndarraytypes.h -- the integer types correspond exactly to native C type names, so even if int and long are the same on some platform, they have distinct Numpy type codes. The NPY_INT16, 32, NPY_INTP etc. are aliases to the basic types, and on some platforms these can overlap.

I think there are opportunities for similar int vs. long fixes in the rest of ndimage -- along these lines:
https://github.com/pv/scipy-work/tree/bug/ndimage-typenum-fix (would need some tests, though, and testing).

@stefanv
Copy link
Member

stefanv commented Aug 29, 2011

Sorry, tested on my changes and merged Matthew's. The #ifdefs I removed fixed the compilation. Will have to check if that is the correct solution or not.

@stefanv
Copy link
Member

stefanv commented Aug 29, 2011

Ah, I see you fixed it. Thanks :)

@matthew-brett
Copy link
Contributor Author

Thanks - works fine for me, thanks for the explanation.

mckib2 pushed a commit that referenced this pull request Dec 16, 2021
ENH: update PROPACK submodule commit hash
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.

None yet

3 participants