Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve use of max_iter_harmony + max_iter_kmeans #8

Merged
merged 1 commit into from
Aug 11, 2020

Conversation

pinin4fjords
Copy link
Contributor

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
Copy link
Owner

slowkow commented Aug 11, 2020

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

@slowkow slowkow merged commit 9556f8c into slowkow:master Aug 11, 2020
slowkow added a commit that referenced this pull request Aug 11, 2020
This fixes a bug in pull request #8 that caused this error:

    AttributeError: 'Harmony' object has no attribute 'max_iter_harmony'
@slowkow
Copy link
Owner

slowkow commented Aug 11, 2020

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
Copy link
Contributor Author

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
Copy link
Owner

slowkow commented Aug 11, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants