Skip to content
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

More options for in-memory objects #17

Merged
merged 1 commit into from
May 24, 2021
Merged

Conversation

david-cortes
Copy link
Contributor

This PR introduces several changes in order to work better with in-memory data:

  • Makes the functions accept sparse matrix objects as inputs.
  • Leaves the option to store the fitted matrices in the model object in memory instead of creating a file, and access them directly for easier retrieval.
  • Changes the default options for some arguments to avoid accidentally creating files where it's not intended.

@yixuan
Copy link
Owner

yixuan commented Apr 25, 2021

This looks great. Thank you @david-cortes! I'll find a time to take a look and merge it.

@yixuan yixuan merged commit 3c52186 into yixuan:master May 24, 2021
@yixuan
Copy link
Owner

yixuan commented May 24, 2021

Hi David, I have merged your PR with a few changes:

  1. The matrix data source is now handled by a new function data_matrix(), to keep the interface clean and consistent.
  2. The model can export P and Q to files even if the model matrices are stored in memory. (In the original PR it will stop.)
  3. I finally choose not to explicitly output a matrix in predict(), since it makes the interface inconsistent. Instead, the user can easily construct a predicted sparse matrix by calling Matrix::sparseMatrix. (But maybe we can write a new output format such as output_matrix()?)

Again, I apologize for the late reply and thank you for the great work! I have added you as an author in the upcoming new version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants