Improve use of max_iter_harmony + max_iter_kmeans

slowkow / harmonypy

🎼 Integrate multiple high-dimensional datasets with fuzzy k-means and locally linear adjustments.

https://portals.broadinstitute.org/harmony/

GNU General Public License v3.0

192 stars 22 forks source link

Improve use of max_iter_harmony + max_iter_kmeans #8

Closed pinin4fjords closed 4 years ago

pinin4fjords commented 4 years ago

Hi @slowkow - not a Python pro, but I believe there are issues with how the max_iter parameters are being used in harmonypy. Proposing the following fixes with reference to RunHarmony.Rd:

Expose max_iter_harmony as top-level argument
Set max_iter_kmeans default to that of original harmony
Pass max_iter_harmony to harmonize() to dictate number of harmony runs, rather than max_iter_kmeans (which dictates the number of clusterings within each harmony run).

Obviously apologies if I've misunderstood somewhere.

slowkow commented 4 years ago

Thanks for catching this! I will take a closer look soon, but it looks like your changes are correct.

slowkow commented 4 years ago

Thanks for the pull request! I fixed a small bug in your code, but that was my fault for the lack of automated testing.

When time allows... I would love to set up automated tests for every commit and pull request.

pinin4fjords commented 4 years ago

Ahh, apologies for missing that, thanks @slowkow .

Will you release soon? We're Conda-centric and that will tricker the automatic Bioconda process to produce a new version.

slowkow commented 4 years ago

Released! https://github.com/slowkow/harmonypy/releases/tag/v0.0.5