compiler: fix temp/array alloc - #2664
Conversation
Sorry, something went wrong.
Codecov ReportAttention: Patch coverage is 76.31579% with 9 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## main #2664 +/- ##
==========================================
- Coverage 91.98% 91.30% -0.69%
==========================================
Files 245 245
Lines 48811 48831 +20
Branches 4307 4308 +1
==========================================
- Hits 44901 44583 -318
- Misses 3215 3536 +321
- Partials 695 712 +17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry.
|
Sorry, something went wrong.
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Sorry, something went wrong.
|
|
||
|
|
||
| @pytest.mark.parametrize('order', [2, 4]) | ||
| def test_int64_array(order): |
There was a problem hiding this comment.
Nitpick: test_array_w_int64_index perhaps? The current test name sounds like the Array contains int64 data
Sorry, something went wrong.
|
|
||
| ULONG = cast(np.uint64) | ||
| UINTP = cast(np.uint32, '*') | ||
| LONG = cast(np.int64) |
There was a problem hiding this comment.
uber-nitpicking: makes me thing we could rather use as_ulong since it's a size which is definitely positive
Sorry, something went wrong.
There was a problem hiding this comment.
We could, but annoying to red-uodate those notebooks. I don't think long is much of an issue and unsigned long everywhere will just make it more verbose
Sorry, something went wrong.
|
|
||
| def as_long(expr): | ||
| """ | ||
| Convert an expression and its symbolic args to a long integer. |
There was a problem hiding this comment.
Actually, I just checked, as long as one of the operands is cast to long, you're good to go. IOW, you don't need to cast all operands to long. Which might improve verbosity.
So technically we might get away with simply just changing these lines
SizeOf(obj._C_typedata)*obj.size
into
ULONG(SizeOf(obj._C_typedata))*obj.size
Sorry, I only thought about this now 😬
Sorry, something went wrong.
There was a problem hiding this comment.
No it doesn't, I actually tried with the JIT backdoor and the obj.size will evaluate as int first then converted to long so if it overflows then the conversion will not do anything
Sorry, something went wrong.
| self.gpu_create = kwargs['options']['gpu-create'] | ||
| self.pmode = kwargs['options'].get('place-transfers') | ||
| def __init__(self, options=None, **kwargs): | ||
| self.gpu_fit = options['gpu-fit'] |
There was a problem hiding this comment.
Won't this fail if options is not supplied, since it will try to access a NoneType object as if it were a dict? Shouldn't the default be options={'gpu-fit': False, 'gpu-create': False} or similar?
Sorry, something went wrong.
There was a problem hiding this comment.
no they are guaranteed to be there by construction
Sorry, something went wrong.
There was a problem hiding this comment.
I suppose it's only a kwarg for readability then? In practice it is mandatory (and thus more like a positional arg)?
Sorry, something went wrong.
There was a problem hiding this comment.
IIRC, it's because all passes accept just one arg (the list of clusters, or the IET) and multiple kwargs. But anyway, legacy might be hitting us here. Anyway, nitpicking
Sorry, something went wrong.
Fix allocation size with index-mode set to long