Improve performance #968
Improve performance #968
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #968 +/- ##
=======================================
Coverage 84.83% 84.83%
=======================================
Files 40 40
Lines 4055 4055
=======================================
Hits 3440 3440
Misses 615 615
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
@davidanthoff this seems fine to me, certainly the collect call. Looking through the use of deepcopy, my guess is that it was a safety to make sure that we don't pass around the model's data by reference, alter it in a DataFrame, and end up with a mutated model object. I think with your change this still wouldn't happen, all the uses of data (other than the one you updated on line 49) mutate the array in some way that allocates a new one, but I could see it feeling fragile.
I think for now it's fine, or we could just deepcopy the DataFrame before returning it ... or add more calls to deepcopy as guard rails but I think that probably just undoes the performance fix you made :)
|
Yes, that |
There are two commits here that are unrelated.
The one about the
deepcopyI'm especially not 100% sure. But I think I followed all code paths, and I think previously the deep copy that was created originally was then actually never (except for the one case where I added adeepcopyreturned to user, but was essentially always a temporary copy, in which case it seems unnecessary to me.