Small improvement to _discrete_log_pohlig_hellman()

[teschlg]

Compute order and its factoring in one pass

Brief description of what is fixed or changed

The function discrete_log_pohlig_hellman currently first computes the order of the
group element and then factors the result. Since computing the order is done
by factoring the group order, this can be done along the way eliminating the need
of another factorization. To this end, the code from n_order() is slightly adapted.

• ntheory
  • .discrete_log now computes the order and its factorization in one pass

[sympy-bot]

✅

Hi, I am the SymPy bot. I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

• ntheory
  • .discrete_log now computes the order and its factorization in one pass (#26487 by @teschlg)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.13.

Click here to see the pull request description that was parsed.

Compute order and its factoring in one pass

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### Brief description of what is fixed or changed

The function discrete_log_pohlig_hellman currently first computes the order of the
group element and then factors the result. Since computing the order is done
by factoring the group order, this can be done along the way eliminating the need
of another factorization. To this end, the code from n_order() is slightly adapted.

<!-- BEGIN RELEASE NOTES -->
* ntheory
  * .discrete_log now computes the order and its factorization in one pass
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

[haru-44]

Thank you for your contribution.

I think the idea is great, but when a user calls discrete_log, the order in _discrete_log_pohlig_hellman is never None. Such an improvement would be better made in discrete_log.

As a point about the code, from collections import defaultdict should be at the top of the file. There is no need to follow the bad precedent. Also, the spacing rules are not consistent, such as f= defaultdict(int) or i=0,order = 1. Unless there is a special reason, it is better to write i = 0 to align with other codes. Also, I think dict is sufficient for f, not defaultdict.

[teschlg]

Thanks for your feedback! I will make the suggested changes.

[github-actions]

Benchmark results from GitHub Actions

Lower numbers are good, higher numbers are bad. A ratio less than 1
means a speed up and greater than 1 means a slowdown. Green lines
beginning with + are slowdowns (the PR is slower then master or
master is slower than the previous release). Red lines beginning
with - are speedups.

Significantly changed benchmark results (PR vs master)

Significantly changed benchmark results (master vs previous release)

| Change   | Before [2487dbb5]    | After [82e59a73]    |   Ratio | Benchmark (Parameter)                                                |
|----------|----------------------|---------------------|---------|----------------------------------------------------------------------|
| -        | 70.3±1ms             | 45.7±0.5ms          |    0.65 | integrate.TimeIntegrationRisch02.time_doit(10)                       |
| -        | 70.7±0.7ms           | 44.4±0.3ms          |    0.63 | integrate.TimeIntegrationRisch02.time_doit_risch(10)                 |
| +        | 18.6±0.1μs           | 31.0±0.5μs          |    1.67 | integrate.TimeIntegrationRisch03.time_doit(1)                        |
| -        | 5.28±0.05ms          | 2.92±0.06ms         |    0.55 | logic.LogicSuite.time_load_file                                      |
| -        | 73.3±0.4ms           | 28.6±0.1ms          |    0.39 | polys.TimeGCD_GaussInt.time_op(1, 'dense')                           |
| -        | 26.0±0.1ms           | 17.0±0.1ms          |    0.66 | polys.TimeGCD_GaussInt.time_op(1, 'expr')                            |
| -        | 74.0±0.7ms           | 28.8±0.05ms         |    0.39 | polys.TimeGCD_GaussInt.time_op(1, 'sparse')                          |
| -        | 255±1ms              | 125±0.5ms           |    0.49 | polys.TimeGCD_GaussInt.time_op(2, 'dense')                           |
| -        | 254±1ms              | 125±0.8ms           |    0.49 | polys.TimeGCD_GaussInt.time_op(2, 'sparse')                          |
| -        | 653±3ms              | 372±1ms             |    0.57 | polys.TimeGCD_GaussInt.time_op(3, 'dense')                           |
| -        | 657±3ms              | 376±1ms             |    0.57 | polys.TimeGCD_GaussInt.time_op(3, 'sparse')                          |
| -        | 490±7μs              | 287±2μs             |    0.59 | polys.TimeGCD_LinearDenseQuadraticGCD.time_op(1, 'dense')            |
| -        | 1.82±0.01ms          | 1.05±0.01ms         |    0.58 | polys.TimeGCD_LinearDenseQuadraticGCD.time_op(2, 'dense')            |
| -        | 9.41±0.03ms          | 3.08±0.05ms         |    0.33 | polys.TimeGCD_LinearDenseQuadraticGCD.time_op(3, 'dense')            |
| -        | 449±0.7μs            | 231±1μs             |    0.51 | polys.TimeGCD_QuadraticNonMonicGCD.time_op(1, 'dense')               |
| -        | 1.47±0ms             | 683±2μs             |    0.47 | polys.TimeGCD_QuadraticNonMonicGCD.time_op(2, 'dense')               |
| -        | 4.98±0.03ms          | 1.66±0.01ms         |    0.33 | polys.TimeGCD_QuadraticNonMonicGCD.time_op(3, 'dense')               |
| -        | 377±1μs              | 208±2μs             |    0.55 | polys.TimeGCD_SparseGCDHighDegree.time_op(1, 'dense')                |
| -        | 2.45±0.01ms          | 1.22±0.01ms         |    0.5  | polys.TimeGCD_SparseGCDHighDegree.time_op(3, 'dense')                |
| -        | 10.2±0.2ms           | 4.35±0.01ms         |    0.42 | polys.TimeGCD_SparseGCDHighDegree.time_op(5, 'dense')                |
| -        | 360±3μs              | 170±1μs             |    0.47 | polys.TimeGCD_SparseNonMonicQuadratic.time_op(1, 'dense')            |
| -        | 2.52±0.02ms          | 889±3μs             |    0.35 | polys.TimeGCD_SparseNonMonicQuadratic.time_op(3, 'dense')            |
| -        | 9.57±0.03ms          | 2.62±0.02ms         |    0.27 | polys.TimeGCD_SparseNonMonicQuadratic.time_op(5, 'dense')            |
| -        | 1.03±0.01ms          | 431±3μs             |    0.42 | polys.TimePREM_LinearDenseQuadraticGCD.time_op(3, 'dense')           |
| -        | 1.72±0.01ms          | 504±1μs             |    0.29 | polys.TimePREM_LinearDenseQuadraticGCD.time_op(3, 'sparse')          |
| -        | 5.89±0.04ms          | 1.79±0ms            |    0.3  | polys.TimePREM_LinearDenseQuadraticGCD.time_op(5, 'dense')           |
| -        | 8.49±0.07ms          | 1.52±0.02ms         |    0.18 | polys.TimePREM_LinearDenseQuadraticGCD.time_op(5, 'sparse')          |
| -        | 282±1μs              | 64.4±0.5μs          |    0.23 | polys.TimePREM_QuadraticNonMonicGCD.time_op(1, 'sparse')             |
| -        | 3.42±0.08ms          | 399±1μs             |    0.12 | polys.TimePREM_QuadraticNonMonicGCD.time_op(3, 'dense')              |
| -        | 3.98±0.05ms          | 283±2μs             |    0.07 | polys.TimePREM_QuadraticNonMonicGCD.time_op(3, 'sparse')             |
| -        | 6.98±0.04ms          | 1.28±0.01ms         |    0.18 | polys.TimePREM_QuadraticNonMonicGCD.time_op(5, 'dense')              |
| -        | 8.70±0.05ms          | 847±5μs             |    0.1  | polys.TimePREM_QuadraticNonMonicGCD.time_op(5, 'sparse')             |
| -        | 5.09±0.01ms          | 2.97±0.01ms         |    0.58 | polys.TimeSUBRESULTANTS_LinearDenseQuadraticGCD.time_op(2, 'sparse') |
| -        | 12.1±0.03ms          | 6.53±0.02ms         |    0.54 | polys.TimeSUBRESULTANTS_LinearDenseQuadraticGCD.time_op(3, 'dense')  |
| -        | 22.6±0.2ms           | 9.06±0.05ms         |    0.4  | polys.TimeSUBRESULTANTS_LinearDenseQuadraticGCD.time_op(3, 'sparse') |
| -        | 5.28±0.02ms          | 886±4μs             |    0.17 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(1, 'sparse')    |
| -        | 12.6±0.04ms          | 7.06±0.01ms         |    0.56 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(2, 'sparse')    |
| -        | 102±2ms              | 25.8±0.06ms         |    0.25 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(3, 'dense')     |
| -        | 167±3ms              | 54.0±0.3ms          |    0.32 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(3, 'sparse')    |
| -        | 174±1μs              | 114±0.4μs           |    0.65 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(1, 'dense')      |
| -        | 359±1μs              | 218±2μs             |    0.61 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(1, 'sparse')     |
| -        | 4.22±0.03ms          | 843±2μs             |    0.2  | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(3, 'dense')      |
| -        | 5.25±0.03ms          | 382±1μs             |    0.07 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(3, 'sparse')     |
| -        | 20.0±0.2ms           | 2.81±0.01ms         |    0.14 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(5, 'dense')      |
| -        | 22.8±0.2ms           | 627±2μs             |    0.03 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(5, 'sparse')     |
| -        | 480±2μs              | 137±1μs             |    0.29 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(1, 'sparse') |
| -        | 4.66±0.02ms          | 621±3μs             |    0.13 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(3, 'dense')  |
| -        | 5.30±0.03ms          | 139±0.6μs           |    0.03 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(3, 'sparse') |
| -        | 13.2±0.2ms           | 1.31±0.01ms         |    0.1  | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(5, 'dense')  |
| -        | 13.9±0.1ms           | 142±3μs             |    0.01 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(5, 'sparse') |
| -        | 134±2μs              | 74.4±0.2μs          |    0.56 | solve.TimeMatrixOperations.time_rref(3, 0)                           |
| -        | 247±2μs              | 88.5±0.2μs          |    0.36 | solve.TimeMatrixOperations.time_rref(4, 0)                           |
| -        | 24.5±0.2ms           | 10.4±0.2ms          |    0.42 | solve.TimeSolveLinSys189x49.time_solve_lin_sys                       |
| -        | 28.7±0.2ms           | 15.7±0.1ms          |    0.55 | solve.TimeSparseSystem.time_linsolve_Aaug(20)                        |
| -        | 55.4±0.1ms           | 25.4±0.2ms          |    0.46 | solve.TimeSparseSystem.time_linsolve_Aaug(30)                        |
| -        | 28.7±0.2ms           | 15.5±0.02ms         |    0.54 | solve.TimeSparseSystem.time_linsolve_Ab(20)                          |
| -        | 54.7±0.2ms           | 25.1±0.07ms         |    0.46 | solve.TimeSparseSystem.time_linsolve_Ab(30)                          |

Full benchmark results can be found as artifacts in GitHub Actions
(click on checks at the top of the PR).

[teschlg]

@haru-44 Let me know what you think about the new version. Thanks.