DAME-FLAME-Python-Package icon indicating copy to clipboard operation
DAME-FLAME-Python-Package copied to clipboard

Replacing some for loops to native pandas functions for increased performace

Open GenericUser127 opened this issue 2 years ago • 6 comments

Also bumping Numpy to a more secure version

GenericUser127 avatar Aug 21 '23 16:08 GenericUser127

Hi @GenericUser127 thanks for your contributions to this open source package! I'm pretty excited to see that you found an area for a potential speed-up and I'll look into it before I approve to make sure it's not breaking tests and is keeping the same functionality. I don't know if I want to bump up the numpy version just because I like the idea of backwards compatibility and older users being able to use the package -- but if you have other thoughts I would be happy to discuss further?

nehargupta avatar Aug 21 '23 21:08 nehargupta

Hi @nehargupta , thanks for the reply.

Please do verify for yourself but we did run the included unit tests on these changes and confirmed they still all passed.

Regarding the Numpy version bump, that was the result of running a Mend check against it and it identifying a vulnerability.

As it is not required for this functionality change, I can remove it from this PR if that is what you prefer?

Thanks!

GenericUser127 avatar Aug 24 '23 09:08 GenericUser127

Hi @nehargupta , is this ready to merge? Thank you!

brianwarner avatar Nov 02 '23 19:11 brianwarner

Hi @nehargupta, I was doing some maintenance and merged master into the branch. Is this ready to merge? Thanks!

brianwarner avatar Nov 30 '23 00:11 brianwarner

Hi @brianwarner Thanks for your work on this! Yeah I think I would prefer the numpy bump in a separate PR but the .loc instead of the for loop is a good idea at quick glance (I still need to run the tests though), thanks for noticing those and for your work!

nehargupta avatar Dec 19 '23 16:12 nehargupta

Updated the PR to remove the Numpy version bump @nehargupta , please let me know if there is anything else you need

GenericUser127 avatar Jan 31 '24 07:01 GenericUser127