OpenMS / OpenMS

The codebase of the OpenMS project
https://www.openms.de
Other
478 stars 318 forks source link

pyopenms how to deal with uint64 ? #5625

Open timosachsenberg opened 3 years ago

timosachsenberg commented 3 years ago
in 20 21 file_description.size = feature_map.size() ---> 22 file_description.unique_id = feature_map.getUniqueId() 23 print(file_description.unique_id) 24 file_descriptions[i] = file_description pyopenms\pyopenms_1.pyx in pyopenms.pyopenms_1.ColumnHeader.unique_id.__set__() OverflowError: Python int too large to convert to C unsigned long
timosachsenberg commented 3 years ago

On windows

jpfeuffer commented 3 years ago

I would say don't use unsigned long since it is platform dependent. Just use our UInt64 typedef.

timosachsenberg commented 3 years ago

Ok thanks. Will check where this type comes from and how to change it

timosachsenberg commented 3 years ago

ok what about size? this is wrong? https://github.com/OpenMS/OpenMS/blob/develop/src/pyOpenMS/pxds/UniqueIdInterface.pxd#L13

hroest commented 3 years ago

See

https://github.com/OpenMS/OpenMS/blob/5c51ad02b833986049f250c1cb46fe02b77b79cc/src/pyOpenMS/pxds/Types.pxd#L25

so Size should be size_t - so that makes me wonder if the Windows version is 32 bit? Otherwise, if its compiled for 64 bit then I would assume that size_t is 64 bit?

timosachsenberg commented 3 years ago

on the left side of the assignment we have a UInt64 https://github.com/OpenMS/OpenMS/blob/develop/src/pyOpenMS/pxds/ConsensusMap.pxd#L28 on the right side a Size type

jpfeuffer commented 3 years ago

I meant more this: https://github.com/OpenMS/OpenMS/blob/5c51ad02b833986049f250c1cb46fe02b77b79cc/src/pyOpenMS/pxds/ConsensusMap.pxd#L35

timosachsenberg commented 3 years ago

aah... k will change

jpfeuffer commented 3 years ago

let's hope autowrap knows about typedefs and does not complain about maps with wrapped key+value

timosachsenberg commented 3 years ago

according to uwe that is a wrapper around Map https://github.com/OpenMS/OpenMS/blob/5c51ad02b833986049f250c1cb46fe02b77b79cc/src/pyOpenMS/addons/ConsensusMap.pyx#L23 but we use std::map can't we then not get rid of https://github.com/OpenMS/OpenMS/blob/5c51ad02b833986049f250c1cb46fe02b77b79cc/src/pyOpenMS/addons/ConsensusMap.pyx#L7-L25

jpfeuffer commented 3 years ago

Unless autowrap+msvc complained just about the potential type incompatibility, I would keep the extension and change the types there as well But you could try.

timosachsenberg commented 3 years ago

let's see if this works https://github.com/OpenMS/OpenMS/pull/5634

jpfeuffer commented 3 years ago

Did you test it on windows? Because our night lies still fail because of numpy

jpfeuffer commented 2 years ago

@timosachsenberg didn't you recently fix GitHub actions? Also pyopenms?

hroest commented 2 years ago

@timosachsenberg is there a way you can test this, eg with large integers in Python?

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.