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

Fix quantiles() compatibility issue #1346

Merged

Conversation

PulpCattel
Copy link
Member

Fixes #1340

statistics.quantiles() is new in Python 3.8, replace it with custom implementation to preserve compatibility with older Python versions.

Tested a bit with:

from statistics import quantiles
from random import random, randint

def get_percentiles(data):
    n = len(data)
    if n < 2:
        raise ValueError("Not enough data, at least two data points required")
    data = sorted(data)
    m = n - 1
    result = []
    for i in range(1, 100):
        j, delta = divmod(i * m, 100)
        interpolated = (data[j] * (100 - delta) + data[j + 1] * delta) / 100
        result.append(interpolated)
    return result
    
    
for _ in range(10000):
    data = [random() for _ in range(randint(2, 10000))]
    assert get_percentiles(data) == quantiles(data, n=100, method="inclusive")

statistics.quantiles() is new in Python 3.8, replace it with custom implementation to preserve compatibility with older Python versions.
@AdamISZ
Copy link
Member

AdamISZ commented Sep 9, 2022

Great. I will check on a couple old Python versions as a sanity check. Since this has such a low/zero security impact, we don't need to worry too much (the tests are nice though, of course!).

@AdamISZ
Copy link
Member

AdamISZ commented Sep 9, 2022

Edit: oh, well, sorry, I'm having trouble getting Joinmarket's install to work with either py37 or py36. Not sure it's worth putting any more time into it.

@PulpCattel
Copy link
Member Author

I'm having trouble getting Joinmarket's install to work with either py37 or py36

@AdamISZ
I assume so, but just to be clear, the issues you are mentioning are unrelated to this PR/bond_calc.py, right?

@kristapsk
Copy link
Member

kristapsk commented Sep 9, 2022

I should get back to #1218, then CI would at least run test suite for Python 3.6.

EDIT: Sorry, I'm idiot, I added different Bitcoin Core versions, Python was there before.

@kristapsk
Copy link
Member

IMO this can be merged. Test suite passes in CI, @PulpCattel has tested it and most people nowadays likely will have Python 3.8+ anyway.

@AdamISZ AdamISZ merged commit fea33cb into JoinMarket-Org:master Sep 11, 2022
@PulpCattel PulpCattel deleted the fix-quantiles-compatibility-issue branch September 11, 2022 17:12
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.

"cannot import name 'quantiles' from 'statistics'" in Joinmarket 0.9.7
3 participants