-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
PERF: optimize DataFrame.sparse.from_spmatrix performance #32825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
sl = slice(indptr[i], indptr[i + 1]) | ||
idx = IntIndex(n_rows, indices[sl], check_integrity=False) | ||
arr = SparseArray._simple_new(data[sl], idx, dtype) | ||
arrays.append(arr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, also tried with a generator here to avoid pre-allocating all the arrays, but it doesn't really matter. Most of the remaining run time is in DataFrame._from_arrays
.
result.columns = columns | ||
return result | ||
n_rows, n_columns = data.shape | ||
data.sort_indices() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be already done in tocsc
, but that's a scipy implementation detail, and it doesn't really cost much. We need to make sure indices are sorted, since we create IntIndex
with check_integrity=False
that used to check for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment about that inline?
@@ -227,15 +227,24 @@ def from_spmatrix(cls, data, index=None, columns=None): | |||
1 0.0 1.0 0.0 | |||
2 0.0 0.0 1.0 | |||
""" | |||
from pandas import DataFrame | |||
from pandas import DataFrame, SparseDtype | |||
from . import IntIndex, SparseArray | |||
|
|||
data = data.tocsc() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be tocsc(copy=False)
for newer scipy versions, but the gain in performance is minimal anyway with respect to the total runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls
add an asv with a fixed random seed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks nice! The asv would go in benchmarks/sparse.py.
Co-Authored-By: Tom Augspurger <[email protected]>
There is actually already an asv benchmark for this,
that shows comparable improvement to the benchmarks I have done earlier in the issue description. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for confirming that we have an existing benchmark for this.
Yeah, it's not really optimized for the case you know you have all well-behaved arrays you just want to put in a DataFrame. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor nits but otherwise lgtm
data.sort_indices() | ||
indices = data.indices | ||
indptr = data.indptr | ||
data = data.data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you assign to a different variable? This in theory could cause mypy complaints (though ndarray resolving to Any at the moment probably allows it to pass)
Co-Authored-By: William Ayd <[email protected]>
Thanks @WillAyd , I addressed your comments. "Web and docs" CI job failed with the following, but I don't think it's due to this PR,
|
It's not very visible, but the actual error is :
so indeed not from this PR. |
See #32832 |
idx = IntIndex(n_rows, indices[sl], check_integrity=False) | ||
arr = SparseArray._simple_new(array_data[sl], idx, dtype) | ||
arrays.append(arr) | ||
return DataFrame._from_arrays(arrays, columns=columns, index=index) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line will be able to use the verify_integrity=False
from #32858 (or if this PR goes in first, I can add it in that PR)
@jreback Do you have any other comments on this? I addressed your review comment. |
asv_bench/benchmarks/sparse.py
Outdated
@@ -45,8 +45,7 @@ def time_sparse_array(self, dense_proportion, fill_value, dtype): | |||
class SparseDataFrameConstructor: | |||
def setup(self): | |||
N = 1000 | |||
self.arr = np.arange(N) | |||
self.sparse = scipy.sparse.rand(N, N, 0.005) | |||
self.sparse = scipy.sparse.rand(N, N, 0.005, random_state=0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given what we said in the other PR, you can remove this random state again (it's covered by the global setup function which will be run for every benchmark)
Thanks @jorisvandenbossche . After merging in #32858 the benchmarks results are
(previous results in #32825 (comment)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. pls merge master and ping on green.
doc/source/whatsnew/v1.1.0.rst
Outdated
@@ -224,6 +224,9 @@ Performance improvements | |||
- The internal index method :meth:`~Index._shallow_copy` now copies cached attributes over to the new index, | |||
avoiding creating these again on the new index. This can speed up many operations that depend on creating copies of | |||
existing indexes (:issue:`28584`, :issue:`32640`, :issue:`32669`) | |||
- Performance improvement when creating sparse :class:`DataFrame` from | |||
``scipy.sparse`` matrices using the :meth:`DataFrame.sparse.from_spmatrix` | |||
constructor (:issue:`32196`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add the issue number from joris PR as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added PRs by Joris, there have been 5 PRs on this in total.
Ideally another what's new entry should be added since PR's by @jorisvandenbossche also made initialization of extension arrays faster under some conditions, as far as I understand. Though I would rather not add it here, nor am I competent on accurately formulating it.
With #32856 merged, there is a new failure for non unique column names. In particular, pd.DataFrame.sparse.from_spmatrix(mat, columns=['a', 'a']) fails with, pandas/core/arrays/sparse/accessor.py:251: in from_spmatrix
return DataFrame._from_arrays(
pandas/core/frame.py:1919: in _from_arrays
mgr = arrays_to_mgr(
pandas/core/internals/construction.py:77: in arrays_to_mgr
return create_block_manager_from_arrays(arrays, arr_names, axes)
pandas/core/internals/managers.py:1689: in create_block_manager_from_arrays
blocks = form_blocks(arrays, names, axes)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
arrays = [[0, 0.7151164908889105, 0, 0, 0.7790091888007832, 0.8726851819885113, 0.8634012065028459, 0, 0, 0.6582977581693065]
F...13, 0, 0, 0.6245750826925724, 0, 0, 0.6870139075931206]
Fill: 0
IntIndex
Indices: array([1, 2, 3, 6, 9], dtype=int32)
]
names = ['a', 'a'], axes = [['a', 'a'], RangeIndex(start=0, stop=10, step=1)]
def form_blocks(arrays, names, axes):
# put "leftover" items in float bucket, where else?
# generalize?
items_dict = defaultdict(list)
extra_locs = []
names_idx = ensure_index(names)
if names_idx.equals(axes[0]):
names_indexer = np.arange(len(names_idx))
else:
> assert names_idx.intersection(axes[0]).is_unique
E AssertionError any suggestions @jorisvandenbossche ? |
Hmm, it's strange that it would be due to #32856, because it's failing inside |
Yes, sorry, you are right. This failure was already present 2 days ago before I merged master today. |
@@ -228,14 +228,29 @@ def from_spmatrix(cls, data, index=None, columns=None): | |||
2 0.0 0.0 1.0 | |||
""" | |||
from pandas import DataFrame | |||
from pandas._libs.sparse import IntIndex | |||
|
|||
data = data.tocsc() | |||
index, columns = cls._prep_index(data, index, columns) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the problem is that this _pre_index
doesn't actually return Index objects when columns or index is not None (and passing actual Index objects is required when doing verify_integrity=False
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I see. Thanks for confirming that passing duplicate columns names in an Index object is expected to work in _from_arrays
. Will look into it. Thanks Joris!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems to be it, as doing pd.DataFrame.sparse.from_spmatrix(mat, columns=pd.Index(['a', 'a']))
instead of pd.DataFrame.sparse.from_spmatrix(mat, columns=['a', 'a'])
works.
You can add in here a ensure_index
call on both index and columns. (from pandas.core.indexes.api import ensure_index
)
Thanks @jorisvandenbossche and @jreback ! CI is green now. |
thanks @rth very nice |
This optimizes
DataFrame.sparse.from_spmatrix
performance, using an approach proposed by @jorisvandenbossche in #32196 (comment)Bulds on top of #32821 and adds another ~4.5x speed up in addition to that PR. Benchmarks for run time (in seconds) are done by running
pd.DataFrame.sparse.from_spmatrix
on a random sparse CSR array of given n_samples, n_features with a density=0.01:with the benchmarking code below,
Closes #32196 although further optimization might be possible. Around 90% of remaining run time happens in
DataFrame._from_arrays
which goes deeper into pandas internals. Maybe some checks could be disabled there, but that looks less straightforward.