search

almost-matching-exactly / DAME-FLAME-Python-Package

A Python Package providing two algorithms, DAME and FLAME, for fast and interpretable treatment-control matches of categorical data
https://almost-matching-exactly.github.io/DAME-FLAME-Python-Package/
MIT License
56 stars 14 forks source link

Replacing some for loops to native pandas functions for increased performace #65

Open GenericUser127 opened 10 months ago

GenericUser127 commented 10 months ago

Also bumping Numpy to a more secure version

nehargupta commented 10 months ago

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?

GenericUser127 commented 10 months ago

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!

brianwarner commented 8 months ago

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

brianwarner commented 7 months ago

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

nehargupta commented 6 months ago

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!

GenericUser127 commented 5 months ago

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