ENH: ensure yt.utils.funcs.ensure_numpy_array consistently returns a copy#5308
ENH: ensure yt.utils.funcs.ensure_numpy_array consistently returns a copy#5308neutrinoceros wants to merge 1 commit intoyt-project:mainfrom
yt.utils.funcs.ensure_numpy_array consistently returns a copy#5308Conversation
|
Wait, hold up. I'm not sure that we want this behavior. Making copies of every array that goes into cython is potentially going to lead to an enormous amount of memory churn and reallocation. |
yt.utils.funcs.ensure_numpy_array consistently returns a copy
|
to be clear, the only inputs where the existing code doesn't copy already are
|
|
dang, that's brutal. One of the biggest costs of ghost zones, last time I looked, was around copying and recopying arrays and checking that they were in the right units. But I suppose this probably isn't in that category. |
|
so if we don't want this to copy, that's a change in behavior in most cases, and I don't think it's one that's safe to make (people's code is going to break in subtle ways). |
|
nah it's fine, you pointed out that I was interpreting the change incorrectly. |
b307d6b to
688469b
Compare
|
Actually it was easy enough to bring only the backward-compatibility layer from the other PR, so I plan to undraft this after the on-going round of CI. |
688469b to
ade7357
Compare
ade7357 to
f0edd1a
Compare
|
even better; I can actually write this in a way that always copy and doesn't need a backward compatibility layer at all. |
chrishavlin
left a comment
There was a problem hiding this comment.
LGTM, based on the discussion above with @matthewturk I think this one is good to merge.
PR Summary
I noticed this function could be heavily simplified, but also, and perhaps more importantly, that its return type would sometimes be a copy of the input data, but not always. This looks like a footgun so I'd rather make it always return a copy, so as to avoid subtle bugs.
PR Checklist