PERF/BUG: ensure we store contiguous arrays in DataFrame(ndarray) for ArrayManager
State: closed

https://github.com/pandas-dev/pandas/pull/42689 removed an "unwanted" copy, but I actually added this on purpose. This is to ensure we store contiguous 1D arrays by default, which is important for performance reasons. 

Labels:

Comments:
this means we're not respecting the copy=False passed to DataFrame constructor?
> this means we're not respecting the copy=False passed to DataFrame constructor?

Yes, and that was on purpose. Note that the intent was to have this as the _default_ for now, and we can see later if we can provide an (optional) optimization for people that care about preserving a view on the 2D array.

Having this as the default for now also makes it easier to compare benchmarks, otherwise we would need to update all ASVs to add a `copy()` if a dataframe is created from an ndarray.

Also in any case, `df.values` will likely never be a view on the original ndarray `arr` in the standard case, even if `DataFrame(arr)` preserves a view on `arr` (because otherwise we would need to detect that each array is a subsequent slice of a single parent array, which I don't think we want to start doing). So in that sense, I think the `# TODO(ArrayManager) keep view on 2D array?` could be removed anyway now I updated the test to use multiple columns (and not be about the 1-column special case)
We talked before (on the mailing list discussion about BlockManager/ArrayManager) about a potential way to keep `DataFrame(array).values` a fast roundtrip for the ndarray (eg by delaying creating the manager in this case), which would be useful for eg scikit-learn. But I see that as a future potential optimization to look into, if this turns out to be a bottleneck / import use case.

BTW, I could already add some logic to honor an explicit `copy=False` (with the default `copy=None` preserve the "ensure contiguous 1D array" behaviour)
>> this means we're not respecting the copy=False passed to DataFrame constructor?

> Yes, and that was on purpose. Note that the intent was to have this as the default for now, and we can see later if we can provide an (optional) optimization for people that care about preserving a view on the 2D array.

If that's the case, then it can/should be treated the same way as we currently treat dicts passed to the DataFrame constructor (where the default is copy=None).  i.e. it should be made explicit and documented.
> If that's the case, then it can/should be treated the same way as we currently treat dicts passed to the DataFrame constructor (where the default is copy=None). i.e. it should be made explicit and documented.

@jbrockmendel updated the PR to handle this through `copy=None` which is translated into `copy=True` if using array manager and input is ndarray, so using the same way how this keyword is updated for dicts.

I didn't add this to the public _docstring_ of DataFrame because currently ArrayManager is not yet documented in general, but it's now documented for devs in the comments of the the `copy=None` handling. 
> can you verify that the existing BM code paths are unchanged by this in perf 

So in the constructor, the only thing that changes in the BM path is one additional `elif manager == "array"` check, which should not have significant performance impact.
couple small comments, otherwise looks good
The one failure is an unrelated unexpected ResourceWarning