Skip to content

feat: allocate first and then proceed to create matrix from Vec of Ro…#159

Merged
Mec-iS merged 2 commits into
smartcorelib:developmentfrom
morenol:lmm/first_allocate_
Sep 20, 2022
Merged

feat: allocate first and then proceed to create matrix from Vec of Ro…#159
Mec-iS merged 2 commits into
smartcorelib:developmentfrom
morenol:lmm/first_allocate_

Conversation

@morenol

@morenol morenol commented Sep 19, 2022

Copy link
Copy Markdown
Collaborator

…wVectors

I was trying to use this function in a 5000 x 10000 matrix and it took a lot of time and it didn't finish, v_stack seems very expensive to be used here.

Not sure if we should return Option<> or an empty matrix when the vec is empty

@morenol
morenol requested a review from Mec-iS September 19, 2022 14:36
@morenol

morenol commented Sep 19, 2022

Copy link
Copy Markdown
Collaborator Author

Any feedback is appreciated @titoeb

@morenol
morenol marked this pull request as ready for review September 19, 2022 15:42
@titoeb

titoeb commented Sep 20, 2022

Copy link
Copy Markdown
Contributor

Any feedback is appreciated @titoeb

Hey @morenol !
I am very happy you took this up. When I implemented the method, I did not bother too much with performance, but just tried to get the job done with the given interface. I can confirm that my implementation scales very poorly and your implementation works better.

With regard the the return value if the vector of row-vectors is empty we currently rely on it. So therefore I would suggest keeping it for now. If there are some good reasons to change that behavior, that would also be ok for me, but then we also need to change the implementation I referenced.

I cannot review, but from my perspective, there is nothing blocking this PR.

@Mec-iS

Mec-iS commented Sep 20, 2022

Copy link
Copy Markdown
Collaborator

Thanks people!

@Mec-iS
Mec-iS merged commit c21e752 into smartcorelib:development Sep 20, 2022
@morenol
morenol deleted the lmm/first_allocate_ branch September 20, 2022 15:42
morenol added a commit that referenced this pull request Nov 8, 2022
#159)

* feat: allocate first and then proceed to create matrix from Vec of RowVectors
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.

3 participants