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

Fix int conversion #455

Merged
merged 1 commit into from Apr 9, 2024
Merged

Conversation

jorisv
Copy link
Contributor

@jorisv jorisv commented Apr 3, 2024

Close #449

Changes

  • Support new primitive type (char, int8_t, uint8_t, int16_t, uint16_t, uint32_t, uint64_t)
  • Support conversion between signed <-> unsigned integers
  • Support conversion between complex numbers
  • Fix int management on Windows (NPY_INT32 is NPY_LONG, then NPY_INT is never used on Windows)
  • Fix long long management on Mac (NPY_LONGLONG and NPY_LONG are both 64 bits and long and long long are the same type on Mac, then there is only one C++ type that must old two different dtype)
  • Remove casting when converting from Eigen scalar to Numpy scalar.
    This should not remove any functionality since Numpy array are created from the Eigen scalar type

Comparison between numpy dtype in Linux/Mac/Windows

Table comparison

Notes

  • Linux is the only OS to have a 128 bits long double
  • Windows Int32DType is NPY_LONG (7) where is NPY_INT on Mac/Linux (5)
  • Windows Int64DType is NPY_LONGLONG (9) where is NPY_LONG on Mac/Linux (7)
  • Windows long is 32 bits where is 64 bits on Mac/Linux
  • Windows long is not the same type than int and int32_t (but it's also a 32 bits integer)
  • Linux long long is not the same type than long and int64_t (but it's also a 64 bits integer)
  • Mac long is not the same type than int64_t (but it's also a 64 bits integer)
  • On Windows NPY_INT and NPY_LONG target the same size
    • This is an issue because numpy will store python int as NPY_LONG
    • The solution is to convert NPY_LONG to C++ int and long
    • NPY_INT on Windows look like deprecated (impossible to create it from python without calling a deprecated function)
  • On Mac NPY_LONGLONG and NPY_LONG target the same size
    • This is an issue because long and long long are the same C++ type
    • We can't support NPY_LONGLONG because we can only map 1 numpy type to 1 C++ type

TODO

  • Fix on all system
  • Manage mapping of 1 C++ type to many numpy type (via promotion)
  • Manage type promotion
  • Manage tensor and sparse ?
    • Factorize code between tensor and matrix
  • Fix changelog
  • Add PR ref in the code
  • Reactivate scalar -> complex conversion

Linux table

Type NPY_TYPES itemsize c++
BoolDType 0 1 bool
ByteDType 1 1 char/int8_t
CLongDoubleDType 16 32 std::complex
Complex128DType 15 16 std::complex
Complex64DType 14 8 std::complex
Float16DType 23 2 None
Float32DType 11 4 float
Float64DType 12 8 double
Int16DType 3 2 short/int16_t
Int32DType 5 4 int/int32_t
Int64DType 7 8 long/int64_t
Int8DType 1 1 char/int8_t
IntDType 5 4 int/int32_t
LongDType 7 8 long/int64_t
LongDoubleDType 13 16 long double
LongLongDType 9 8 long long
ObjectDType 17 8 None
ShortDType 3 2 short/int16_t
UByteDType 2 1 unsigned char/uint8_t
UInt16DType 4 2 unsigned short/uint16_t
UInt32DType 6 4 unsigned int/uint32_t
UInt64DType 8 8 unsigned long/uint64_t
UInt8DType 2 1 unsigned char/uint8_t
UIntDType 6 4 unsigned int/uint32_t
ULongDType 8 8 unsigned long/uint64_t
ULongLongDType 10 8 unsigned long long
UShortDType 4 2 unsigned short/uint16_t

Mac table

Type NPY_TYPES itemsize c++
BoolDType 0 1 bool
ByteDType 1 1 char/int8_t
CLongDoubleDType 16 16 std::complex
Complex128DType 15 16 std::complex
Complex64DType 14 8 std::complex
Float16DType 23 2 None
Float32DType 11 4 float
Float64DType 12 8 double
Int16DType 3 2 short/int16_t
Int32DType 5 4 int/int32_t
Int64DType 7 8 long/int64_t
Int8DType 1 1 char/int8_t
IntDType 5 4 int/int32_t
LongDType 7 8 long/int64_t
LongDoubleDType 13 8 long double
LongLongDType 9 8 long long
ObjectDType 17 8 None
ShortDType 3 2 short/int16_t
UByteDType 2 1 unsigned char/uint8_t
UInt16DType 4 2 unsigned short/uint16_t
UInt32DType 6 4 unsigned int/uint32_t
UInt64DType 8 8 unsigned long/uint64_t
UInt8DType 2 1 unsigned char/uint8_t
UIntDType 6 4 unsigned int/uint32_t
ULongDType 8 8 unsigned long/uint64_t
ULongLongDType 10 8 unsigned long long
UShortDType 4 2 unsigned short/uint16_t

Windows table

Type NPY_TYPES itemsize c++
BoolDType 0 1 bool
ByteDType 1 1 char/int8_t
CLongDoubleDType 16 16 std::complex
Complex128DType 15 16 std::complex
Complex64DType 14 8 std::complex
Float16DType 23 2 None
Float32DType 11 4 float
Float64DType 12 8 double
Int16DType 3 2 short/int16_t
Int32DType 7 4 int/int32_t
Int64DType 9 8 long/int64_t
Int8DType 1 1 char/int8_t
IntDType 5 4 int/long/int32_t
LongDType 7 4 int/long/int32_t
LongDoubleDType 13 8 long double
LongLongDType 9 8 long long
ObjectDType 17 8 None
ShortDType 3 2 short/int16_t
UByteDType 2 1 unsigned char/uint8_t
UInt16DType 4 2 unsigned short/uint16_t
UInt32DType 8 4 unsigned int/uint32_t
UInt64DType 10 8 unsigned long/uint64_t
UInt8DType 2 1 unsigned char/uint8_t
UIntDType 6 4 unsigned int/uint32_t
ULongDType 8 4 unsigned long/uint64_t
ULongLongDType 10 8 unsigned long long
UShortDType 4 2 unsigned short/uint16_t

@jorisv jorisv self-assigned this Apr 3, 2024
@hrp2-14
Copy link

hrp2-14 commented Apr 3, 2024

Hi ! This project doesn't usually accept pull requests on the main branch.
If this wasn't intentionnal, you can change the base branch of this PR to devel
(No need to close it for that). Best, a bot.

@jorisv jorisv changed the base branch from master to devel April 3, 2024 12:28
jcarpent
jcarpent previously approved these changes Apr 3, 2024
Copy link
Contributor

@jcarpent jcarpent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jorisv Amazing. Thanks a lot for the big fix which will help to better support Windows applications

@jorisv jorisv force-pushed the topic/fix_int_conversion branch 2 times, most recently from 91d0e61 to cacaf18 Compare April 5, 2024 10:10
ManifoldFR
ManifoldFR previously approved these changes Apr 8, 2024
Copy link
Member

@ManifoldFR ManifoldFR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scrumptious

@jorisv jorisv marked this pull request as ready for review April 8, 2024 19:47
int_conv: Add more integer type binding and add a unit test

changelog: Add changelog entry

int_conv: Manage int8_t and long on Windows

int_conv: Try to use stdint type to manage long hack on windows

int_conv: Fix Windows build

int_conv: Try to fix Mac build and don't compile twice 64 bits integer on Mac and Windows

int_conv: Don't use np.dtypes (introduced in numpy 1.25)

int_conv: Test long and int difference in unit test

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

int_conv: Expose long in MacOS

int_conv: Don't use deprecated type np.intc

int_conv: Fix Mac build

int_conv: Remove int128 (doesn't exists)

int_conv: Disable long long test on Mac because the type is not reachable

int_conv: Begin to work on promotion

int_conv: Add unsigned promotion

int_conv: Manage complex promotion

int_conv: Split matrix binding to avoid memory issue while building

int_conv: Split decomposition binding to avoid memory issue while building

int_conv: Use clongdouble instead of complex256

int_conv: Avoid out of heap space error on Windows

int_conv: Fix test for Mac

int_conv: Try to manage longlong on Mac and int on Windows, also reduce compile time

int_conv: Manage Windows and Mac int and long long when casting

int_conv: Allow scalar to complex conversion

int_conv: Manage casting from numpy to eigen with tensor

int_conv: Add changelog entries

int_conv: Remove conversion from Eigen to Numpy

int_conv: Add reference to this PR

int_conv: Activate SciPy tests by default
@jorisv jorisv enabled auto-merge April 9, 2024 09:42
Copy link
Member

@ManifoldFR ManifoldFR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the hard work @jorisv !!

@jorisv jorisv merged commit 9a0970d into stack-of-tasks:devel Apr 9, 2024
27 checks passed
@jorisv jorisv deleted the topic/fix_int_conversion branch April 9, 2024 11:51
nim65s added a commit to nim65s/robotpkg that referenced this pull request Apr 22, 2024
    ## [3.5.0] - 2024-04-14

    ### Added
    - Allow use of installed JRL-cmakemodule (stack-of-tasks/eigenpy#446)
    - Support of Numpy 2.0.0b1 (stack-of-tasks/eigenpy#448)
    - Support new primitive type (char, int8_t, uint8_t, int16_t, uint16_t, uint32_t, uint64_t) ()stack-of-tasks/eigenpy#455)
    - Support conversion between signed <-> unsigned integers (stack-of-tasks/eigenpy#455)
    - Support conversion between complex numbers (stack-of-tasks/eigenpy#455)

    ### Fixed
    - Fix unit test build in C++11 (stack-of-tasks/eigenpy#442)
    - Fix unit test function signature [#443](stack-of-tasks/eigenpy#443)
    - Fix CMake export (stack-of-tasks/eigenpy#446)
    - Fix `int` management on Windows (stack-of-tasks/eigenpy#455)
    - Fix `long long` management on Mac (stack-of-tasks/eigenpy#455)
    - Allow to run test in the build directory on Windows (stack-of-tasks/eigenpy#457)

    ### Removed
    - Remove casting when converting from Eigen scalar to Numpy scalar.
      This should not remove any functionality since Numpy array are created from the Eigen scalar type
      (stack-of-tasks/eigenpy#455)
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.

SimplicialLDLT Error on Windows
4 participants