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

Faster runtime of predict_win and predict_draw #48

Merged
merged 4 commits into from
Mar 10, 2022

Conversation

philihp
Copy link
Contributor

@philihp philihp commented Mar 1, 2022

Description of Changes

  • Adds some unit tests that I have for the Javascript library, to prevent any regressions.
  • Tells itertools.permutations to give permutation pairs, rather than all full permutations
  • Additionally, the length of permutation_pairs is shorter, and that simplifies the denominator to just a triangular number.

I noticed that for teams of ABCD, your code as written was finding all permutations (ABCD, ABDC, ACBD, ACDB, ...) and then only using the first two from the permutation, which for n>=4 teams, this causes the same pairings to be calculated multiple times.

This should reduce runtime from O(n^n) to O(n^2).

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under openskill.py's MIT license.

I certify the above statement is true and correct:
Philihp Busby

openskill/rate.py Outdated Show resolved Hide resolved
@vivekjoshy vivekjoshy self-requested a review March 1, 2022 04:38
Copy link
Owner

@vivekjoshy vivekjoshy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please format the code with black . so that the checks can pass. Can you also add two spaces between the test functions for predicting wins as per PEP 8 E302.

@philihp
Copy link
Contributor Author

philihp commented Mar 9, 2022

Sure thing! (sorry Python is a new language to me, I'll see what I can do)

* main:
  Bump furo from 2022.2.23 to 2022.3.4
  Fix wrong variable in benchmarks
@philihp philihp requested a review from vivekjoshy March 9, 2022 20:00
@codecov
Copy link

codecov bot commented Mar 10, 2022

Codecov Report

Merging #48 (06bd0dd) into main (3ac32be) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##              main       #48   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           11        11           
  Lines          503       488   -15     
  Branches       110       106    -4     
=========================================
- Hits           503       488   -15     
Impacted Files Coverage Δ
openskill/rate.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ac32be...06bd0dd. Read the comment docs.

@vivekjoshy vivekjoshy merged commit f7b8bd9 into vivekjoshy:main Mar 10, 2022
@philihp philihp deleted the philihp-refactor-predict branch March 21, 2022 17:41
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