WIP, TST: add 32-bit support #2346
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2346 +/- ##
===========================================
- Coverage 89.89% 89.87% -0.03%
===========================================
Files 173 173
Lines 21784 21784
Branches 2861 2861
===========================================
- Hits 19583 19578 -5
- Misses 1609 1613 +4
- Partials 592 593 +1
Continue to review full report at Codecov.
|
|
Initial Azure CI output confirms 32-bit testing with failures related to buffer dtype mismatches as one might expect, with
I'll now push in some of the WIP improvements from my feature branch to demonstrate better results. I should also improve our test code line to produce xml output for the summary, but that can wait for now. |
|
Ok, the improved results are now clearly visible in 32-bit Azure CI run:
@zemanj may be able to help for some of the I'll try to take a look at some other failures as time permits... |
|
I’m partly responsible for cutil too, I’ll have some time in the next week to dig into that.
…
On Sep 7, 2019 at 18:56, <Tyler Reddy ***@***.***)> wrote:
Ok, the improved results are now clearly visible in 32-bit Azure CI run:
= 137 failed, 15264 passed, 84 skipped, 2 xfailed, 2 xpassed, 27939 warnings in 695.13s (0:11:35) =
@zemanj (https://github.com/zemanj) may be able to help for some of the testsuite/MDAnalysisTests/core/test_wrap.py failures--there's an awful lot of integer typing in _cutil.pyx
I'll try to take a look at some other failures as time permits...
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub (#2346?email_source=notifications&email_token=ACGSGB57Z6W2UUWS7BZ25Z3QIRLU3A5CNFSM4IURX232YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6FF4DQ#issuecomment-529161742), or mute the thread (https://github.com/notifications/unsubscribe-auth/ACGSGBYI6JHUNJSKKWMWNWDQIRLU3ANCNFSM4IURX23Q).
|
|
Pushed in some more fixes -- CI should hopefully confirm the new result I get locally for full test suite:
|
There was a problem hiding this comment.
When using ndarray.astype(), it would be cool if you made use of the copy kwarg, i.e., use array.astype(np.int64, copy=False) to avoid unnecessary copy operations.
Related idea:
Since we're often dealing with numpy.intp types which correspond to the C99 type ptrdiff_t (platform-dependent 32 or 64 bit integers), we might think about having separate unique_int32_1d(), unique_int64_1d(), and unique_intp_1d() functions in lib._cutil.
Or, much more convenient: Is it possible to somehow realize a template-based implementation of unique_int_1d() for the different integer types? I don't recall right now how well templates work with cython...
Also related to this issue: IMHO we generally need to consolidate integer types in MDA. For example, we use numpy.intp for atom indices but numpy.int32 for bond indices.
I don't mean that we should change that but we should have proper centralized type definitions for these kind of things instead of having types hard-coded all over the place. I think numpy is a good example in that respect.
|
@zemanj Cython has template like things called fused types.
But really we should just have a single index type like you suggest.
…
On Sep 10, 2019 at 07:48, <Johannes Zeman ***@***.***)> wrote:
@zemanj commented on this pull request.
When using ndarray.astype(), it would be cool if you made use of the copy kwarg, i.e., use array.astype(np.int64, copy=False) to avoid unnecessary copy operations.
Related idea:
Since we're often dealing with numpy.intp types which correspond to the C99 type ptrdiff_t (platform-dependent 32 or 64 bit integers), we might think about having separate unique_int32_1d(), unique_int64_1d(), and unique_intp_1d() functions in lib._cutil.
Or, much more convenient: Is it possible to somehow realize a template-based implementation of unique_int_1d() for the different integer types? I don't recall right now how well templates work with cython...
Also related to this issue: IMHO we generally need to consolidate integer types in MDA. For example, we use numpy.intp for atom indices but numpy.int32 for bond indices.
I don't mean that we should change that but we should have proper centralized type definitions for these kind of things instead of having types hard-coded all over the place. I think numpy is a good example in that respect.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub (#2346?email_source=notifications&email_token=ACGSGBYE2RLGZXCDM5Y25ETQI6XUHA5CNFSM4IURX232YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCEHT5NQ#pullrequestreview-286211766), or mute the thread (https://github.com/notifications/unsubscribe-auth/ACGSGBYAVJ5JQZZ3TGEB4WDQI6XUHANCNFSM4IURX23Q).
|
There was a problem hiding this comment.
So if we have a lot of astype calls it's going to slow down everything, so it's probably better to have things in the correct dtype to begin with (like I tried & failed with in #2352). I don't think we've ever had a user actually require a 32 bit build, it's just downstream maintainers, so I'm not sure it's worth having a slower path that (maybe) later gets fixed.
|
How much is the slow down in practice? Without benchmarks we're just speculating I think. It may be modest. |
|
I think a top-level / initialization variable could be used to detect if you're on a 32-bit platform & if not the astype calls can all be no-ops that return the same type. |
|
Superseded by #2696, so closing. We might want to extract the addition of the 32-bit Linux test to CI from here though. |
Fixes #2342
Currently WIP, with my fork feature branch
fix_32bitreducing total failures to 137 from around 1000 or so. I will first aim to add 32-bit testing to CI.I'm currently stuck trying to get the
testsuite/MDAnalysisTests/core/test_wrap.pytests passing--some of the C++ stuff in_cutil.pyxis not friendly for Python maintainers...Anyway, will aim to get the large number of 32 bit test failures reproduced in CI here for demo purposes, then will try pushing in the WIP improvements to drop the fail count down...