search

motiwari / BanditPAM

BanditPAM C++ implementation and Python package
MIT License
647 stars 38 forks source link

Changing all the instances of kmeds/KMeds to kmedoids/KMedoids. Fix for issue #42 #94

Closed dhaval737 closed 3 years ago

dhaval737 commented 3 years ago

Please review the Pull request for issue #42. These are the steps that I performed to make changes STEPS:

  1. Updated the filename from kmeds_pywrapper.cpp to kmedoids_pywrapper.cpp

  2. Modified all the instances of kmeds to kmedoids in kmedoids_pywrapper.cpp sed -i '' 's/kmeds/kmedoids/g' BanditPAM/src/kmedoids_pywrapper.cpp

  3. Modified all the instances of KMeds to KMedoids in kmedoids_pywrapper.cpp sed -i '' 's/KMeds/KMedoids/g' BanditPAM/src/kmedoids_pywrapper.cpp

  4. Modified all the instances of kmeds to kmedoids in setup.py sed -i '' 's/KMeds/KMedoids/g' BanditPAM/setup.py

  5. Ran the command: sudo pip3 install .

  6. Tested example1 and example2; python examples from Readme

  7. Executed option 2 (Installing Requirements and Building Directly) from the readme file

  8. Ran the below command for test ./BanditPAM -f ../../data/MNIST-1k.csv -k 10 -v 1

    dhavaldangaria@Dhavals-MacBook-Pro src % ./BanditPAM -f ../../data/MNIST-1k.csv -k 10 -v 1 Medoids: 694,800,306,714,324,959,737,527,168,251

  9. Ran the doxygenfrom the docs folder: Before running the “doxygen” command, I cleaned the Docs folder and just kept the Doxyfile in it. Navigated to docs folder and ran the command: rm -rf $(find . -name "*" ! -name "Doxyfile") doxygen Doxyfile

  10. Ran the below commands from the root BanditPAM folder to confirm that there are no more instance of kmeds or KMeds grep -H -r "kmeds". grep -H -r "KMeds" .

motiwari commented 3 years ago

The PR looks much better; thank you for making the suggested changes. I particularly like the detailed instructions about what you did and the test plans to verify the desired changes.

One feedback I would give is to investigate the codemod tool; likely this is more efficient than sed. (not necessary for this PR)

One thing I would add to your test plan is to test the Doxygen documentation. On main and kmeds_clean_up, the documentation appears different (file:///Users/motiwari/Desktop/BanditPAM/docs/html/index.html) -- for example, on this PR, the Main Page is blank and it appears the documentation has been moved to "Related Pages". In addition, it looks like the "Files" dropdown menu now has a "File Members" page which should be deleted. Can you fix?

dhaval737 commented 3 years ago

I have made the changes to the docs as per @motiwari's comment.