sylhare / nprime

:100: Prime numbers algorithms in Python
GNU General Public License v3.0
13 stars 3 forks source link

Docs and misc #25

Closed Erotemic closed 4 years ago

Erotemic commented 4 years ago

With respect to #21, this moves the plt imports inside the functions, so the plot code can be imported at startup time. In the issue you mentioned the submodule logic takes care of this, and you're right, so I can drop 183a2a9 if necessary.

Also in the __init__.py file I noticed there were two definitions of pyprime, one was the submodule and the other was the function. I removed that ambiguity by adding the --nomods flag to mkinit, which only exposes attributes and not the parent modules. However, there is still an ambiguity by exposing the pyprime attribute at the top level. There is now effectively no way to reference the pyprime module, which I believe breaks some of the examples. This can either be fixed by (1) reverting the mkinit change, (2) changing the name of the submodule to something like "core", (3) removing the pyprime function from the __init__.py file or (4) being ok with the loss of that module reference and declaring that core functions should always be used via the top level. Personally I prefer option 2.

Almost lastly, I added several doctests to demonstrate usage of the module. These can be run automatically on TravisCI via installing xdoctest from pypi and including the lines coverage run --source=nprime.pyprime -m xdoctest nprime or if you switched to pytest you could test everything with one command:

pip install pytest pytest-cov xdoctest
pytest --xdoctest --cov=nprime --cov-report xml nprime tests

But either way, the examples should be helpful.

Actually lastly, I slightly improved the efficiency of generate_primes by rougly (1.5x). I stored the sqrt computation in a variable to prevent recalculation and used pythonic iterables rather than index based loops.

def generate_primes_new(upper=0):
    if upper >= 2:
        primes = [2]
        for n in range(3, upper + 1):
            # We only check if n is divided by the previous primes
            sqrt_n = pow(n, 0.5)
            found = None
            # Test if any previous primes divides n
            for p in primes:
                if sqrt_n < p:
                    break
                if n % p == 0:
                    found = p
                    break  # not prime
            if found is None:
                primes.append(n)  # must be prime
    else:
        primes = []
    return primes

def generate_primes_old(upper=0):
    if upper >= 2:
        primes = [2]

        for n in range(3, upper + 1):
            # We only check if n is divided by the previous primes
            k = 0
            sqrt_n = pow(n, 0.5)

            while primes[k] <= sqrt_n and n % primes[k] != 0:
                k += 1

            # if a number has no dividers, last prime[k] of loop is over n's square root
            if sqrt_n < primes[k]:
                primes.append(n)
    else:
        primes = []

    return primes

import timerit
ti = timerit.Timerit(20, bestof=3, verbose=2)

upper = 50000
for timer in ti.reset('old'):
    with timer:
        list(generate_primes_old(upper))

for timer in ti.reset('new'):
    with timer:
        list(generate_primes_new(upper))
Timed old for: 20 loops, best of 3
    time per loop: best=66.917 ms, mean=67.662 ± 1.0 ms
Timed new for: 20 loops, best of 3
    time per loop: best=42.258 ms, mean=43.816 ± 1.0 ms
codecov[bot] commented 4 years ago

Codecov Report

Merging #25 into master will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #25   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           80        85    +5     
=========================================
+ Hits            80        85    +5     
Impacted Files Coverage Δ
nprime/pyprime.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 9b42fdb...86c82a5. Read the comment docs.

sylhare commented 4 years ago

Oh interesting! I'll take a look at all that, thank you very much :)

I really like the example within the documentation, for the pyprime issue it doesn't break the examples in the readme, so if it's only within the demo, or tests then I guess we could make the change to avoid ambiguity (2).

I'll check that out locally tomorrow

sylhare commented 4 years ago

Yup, I like it I'll play with it more and add it to the pipeline. However I am not sure what this >>> # xdoctest: +IGNORE_WANT is for.

Erotemic commented 4 years ago

The >>> # xdoctest: +IGNORE_WANT is a directive that prevents the xdoctest checker from failing if what we "got" is different from what we "want" in the context of the doctest output. This is because the function is probabilistic, so I wouldn't want the dashboards to fail in freak instances where the output is different.

sylhare commented 4 years ago

Oh ok I see, well since there is a good/high chance of those test passing with the default setting of t=100, that won't make them too flaky. 😉