-
Notifications
You must be signed in to change notification settings - Fork 50
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
Use of sparse matrices #239
Comments
Hi 👋 , sorry for the delay, busy week. I really appreciate the clear example, it made things easy to reproduce. So firstly, I'm sorry that the docstring says we accept sparse matrices but, as you point out, we obviously don't because of the way the parameters to You are right that we could probably just add the parameter and some things would start working (and in fact I opened #240 to do so). Sparse values for This said, at a high level, I'm not sure that working with sparse matrices in this way is a great idea: it's highly likely that somewhere along the pipeline the entire data will have to be copied in memory or worse, cast to a dense format. This could be SciKeras converting from DOK -> LIL (TF does not support DOK), or it could be TF converting LIL -> dense tensor (which depends on the ops/layers used). I fear that this could lead to unexpected and tricky to debug results. Maybe you can give #240 a whirl and let me know how performance/memory usage goes with real world data? Also, TensorFlow supports much more advanced data ingestion by means of Thanks! |
Hi, I appreciate the fairly quick response. I would love to check and see if #240 solves the issue. I won't be able to do this with the data I tried it with before, but I will try to find or create a similar dataset in terms of size. I will be travelling for the coming 3 weeks however, so it will be a while untill I have time. As for why I want to use sparse matrices: the first reason is that I have very sparse data, which becomes very large in memory when stored in a dense format. I wanted to use a relatively simple fully connected network for comparison as well. I will admit that I don't know much about sparse inputs to these networks, so I am not sure if the process is memory efficiënt the whole way though. In the end I ran out of time to include it in my thesis anyway. Though I am interested in exploring this use case a bit further. Lastly, I agree that support for a sparse y isnt needed. I have been using a dense array for that in practice too. |
Any chance this could get merged in? I made a quick & dirty fork (just passing |
@mattalhonte-srm does #240 work for you? As in not just run, but actually give you performance (runtime or memory) benefits? |
Hi again, apologies for never coming back to this before. I just performed some tests with some dummy data. For a denisty of 5% (so only 5% of all values in the data are non-zero): Density of 1%: Density of 0.01 %: So as you can see, using sparse matrices does help with reducing general memory usage, especially as datasets get more and more sparse. I do however still feel that the use of tf.data.Datasets would probably be better, but sparce matrices might help in some niche situations. I'm also not sure why I measure a better memory performance, while it seems quite likely that somewhere along the process the sparse matrix will indeed be converted into a dense one. |
Yeah, I get better memory performance - and "not blowing up containers" is a concern for my use case, so this is kind of a dealbreaker feature for me. |
Would you mind posting an example of how you tested this? I just want to make sure this actually works for real world use cases so that we don't add a feature that has no upside and this only serves to confuse users. |
The main way I know is that casting |
Awesome that's about as real world useful as it gets. I think I'll move forward with #240 tomorrow |
@carlogeertse and/or @mattalhonte-srm can you folks take a look at #240 (PR review is appreciated), especially the new tutorial (https://www.adriangb.com/scikeras/refs/pull/240/merge/notebooks/sparse.html) and see if it all looks good to you? If it does, we can merge and cut a release as soon as this weekend. |
Heya! Just tested this and it doesn't work for me - converting it to |
Hi, nice to see someone continue development on a wrapper like this after the people at tensorflow decided to discontinue development on their wrapper.
I have run into an issue with the use of sparse matrices.
In the API documentation it is mentioned that the fit and predict functions from the KerasClassifier wrapper should work with array-like, sparse matrix and dataframe. However, when I use a sparse matrix, I get the following exception:
TypeError: A sparse matrix was passed, but dense data is required. Use X.toarray() to convert to a dense numpy array.
I used the quickstart guide to get a simple reproducable issue. I simply converted the ndarrays in the example into a scipy.sparse coo_matrix:
A potential reason for the issue could be that when validating the inputs via sklearn.utils.check_X_y, the default parameter for accept_sparse is False. See also here
Setting this parameter to true might solve the issue (I will go and test that soon).
I am running this on python=3.7.10, scikit-learn=0.24.2 and tensorflow=2.5.0
The text was updated successfully, but these errors were encountered: