Validate arguments to DMP

[oscarbenjamin]

References to other Issues or PRs

Brief description of what is fixed or changed

Validates the arguments to DMP.

Some tests are fixed to pass domain elements instead of int.

A redundant test function test___hash__ was removed. This was for testing int vs long which might have been relevant in Python 2 but is not needed any more.

A bug in factor for ZZ_I was fixed. I am not sure if this bug would ever lead to incorrect results but an incorrect domain argument was being passed to dmp_ground_primitive meaning that dup_zz_i_factor was returning results over QQ_I rather than ZZ_I.

Other comments

The validation code is slow and should not be used at runtime. I will remove it in a subsequent commit after verifying that all tests have passed with the validation. This is a good example of the sort of thing that should really be handled by a static type checker because runtime type checks are too slow.

Release Notes

NO ENTRY

[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.

• No release notes entry will be added for this pull request.

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

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

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->


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

Validates the arguments to DMP.

Some tests are fixed to pass domain elements instead of int.

A redundant test function `test___hash__` was removed. This was for testing int vs long which might have been relevant in Python 2 but is not needed any more.

A bug in factor for `ZZ_I` was fixed. I am not sure if this bug would ever lead to incorrect results but an incorrect domain argument was being passed to `dmp_ground_primitive` meaning that `dup_zz_i_factor` was returning results over `QQ_I` rather than `ZZ_I`.

#### Other comments

The validation code is slow and should not be used at runtime. I will remove it in a subsequent commit after verifying that all tests have passed with the validation. This is a good example of the sort of thing that should really be handled by a static type checker because runtime type checks are too slow.

#### Release Notes

<!-- Write the release notes for this release below between the BEGIN and END
statements. The basic format is a bulleted list with the name of the subpackage
and the release note for this PR. For example:

* solvers
  * Added a new solver for logarithmic equations.

* functions
  * Fixed a bug with log of integers. Formerly, `log(-x)` incorrectly gave `-log(x)`.

* physics.units
  * Corrected a semantical error in the conversion between volt and statvolt which
    reported the volt as being larger than the statvolt.

or if no release note(s) should be included use:

NO ENTRY

See https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more
information on how to write release notes. The bot will check your release
notes automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
NO ENTRY
<!-- END RELEASE NOTES -->

[hanspi42]

This line is very long.

[hanspi42]

This line is very long.

[hanspi42]

I have looked at it, and apart from a few (I didn't mark all of them) very long lines in the tests it looks good.

I cannot mathematically assess whether " an incorrect domain argument was being passed to dmp_ground_primitive ", but from my review, I can say that it has been corrected consistently in the code.

This would be OK for me; I'd shorten all overlong lines in the test, but maybe it is not really necessary.

[oscarbenjamin]

I'd shorten all overlong lines in the test

Once tests have passed I'll do that. Most of those changes were just done with a regex in vim.

[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 [8059df73] <sympy-1.12^0>   | After [fe959796]    |   Ratio | Benchmark (Parameter)                                                |
|----------|------------------------------------|---------------------|---------|----------------------------------------------------------------------|
| -        | 86.2±0.5ms                         | 56.8±0.2ms          |    0.66 | integrate.TimeIntegrationRisch02.time_doit(10)                       |
| -        | 84.6±1ms                           | 55.9±0.3ms          |    0.66 | integrate.TimeIntegrationRisch02.time_doit_risch(10)                 |
| +        | 22.6±0.1μs                         | 39.2±0.2μs          |    1.73 | integrate.TimeIntegrationRisch03.time_doit(1)                        |
| -        | 6.91±0.03ms                        | 3.78±0ms            |    0.55 | logic.LogicSuite.time_load_file                                      |
| -        | 2.11±0ms                           | 652±2μs             |    0.31 | polys.TimePREM_LinearDenseQuadraticGCD.time_op(3, 'sparse')          |
| -        | 10.3±0.02ms                        | 1.95±0.01ms         |    0.19 | polys.TimePREM_LinearDenseQuadraticGCD.time_op(5, 'sparse')          |
| -        | 368±1μs                            | 81.5±0.3μs          |    0.22 | polys.TimePREM_QuadraticNonMonicGCD.time_op(1, 'sparse')             |
| -        | 4.83±0.01ms                        | 362±0.5μs           |    0.08 | polys.TimePREM_QuadraticNonMonicGCD.time_op(3, 'sparse')             |
| -        | 10.6±0.02ms                        | 1.09±0ms            |    0.1  | polys.TimePREM_QuadraticNonMonicGCD.time_op(5, 'sparse')             |
| -        | 6.23±0ms                           | 3.95±0.01ms         |    0.63 | polys.TimeSUBRESULTANTS_LinearDenseQuadraticGCD.time_op(2, 'sparse') |
| -        | 27.2±0.03ms                        | 11.9±0.01ms         |    0.44 | polys.TimeSUBRESULTANTS_LinearDenseQuadraticGCD.time_op(3, 'sparse') |
| -        | 6.90±0.03ms                        | 1.17±0ms            |    0.17 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(1, 'sparse')    |
| -        | 16.3±0.04ms                        | 9.19±0.02ms         |    0.56 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(2, 'sparse')    |
| -        | 211±0.5ms                          | 70.4±0.05ms         |    0.33 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(3, 'sparse')    |
| -        | 6.36±0.01ms                        | 533±0.9μs           |    0.08 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(3, 'sparse')     |
| -        | 27.8±0.07ms                        | 848±3μs             |    0.03 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(5, 'sparse')     |
| -        | 609±2μs                            | 199±1μs             |    0.33 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(1, 'sparse') |
| -        | 6.49±0.02ms                        | 200±1μs             |    0.03 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(3, 'sparse') |
| -        | 17.2±0.03ms                        | 203±1μs             |    0.01 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(5, 'sparse') |
| -        | 162±0.4μs                          | 89.4±0.4μs          |    0.55 | solve.TimeMatrixOperations.time_rref(3, 0)                           |
| -        | 312±0.4μs                          | 109±0.4μs           |    0.35 | solve.TimeMatrixOperations.time_rref(4, 0)                           |
| -        | 32.0±0.2ms                         | 13.6±0.04ms         |    0.42 | solve.TimeSolveLinSys189x49.time_solve_lin_sys                       |

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

[oscarbenjamin]

The validation code is slow and should not be used at runtime. I will remove it in a subsequent commit

I have commented out the call to the validation code. This is ready for review now.

To be clear the purpose of these changes is to prepare for making it possible to swap out an alternate implementation of DMP that is based on python-flint's faster fmpz_poly etc types.

[hanspi42]

The validation code is slow and should not be used at runtime. I will remove it in a subsequent commit

I have commented out the call to the validation code. This is ready for review now.

To be clear the purpose of these changes is to prepare for making it possible to swap out an alternate implementation of DMP that is based on python-flint's faster fmpz_poly etc types.

It looks fine for this. Just one last question: commenting out _validate_args is not a deprecation, is it? I guess it would only mean that, for wrong args, it would now crash instead of fail in an assertion.

[oscarbenjamin]

It looks fine for this. Just one last question: commenting out _validate_args is not a deprecation, is it? I guess it would only mean that, for wrong args, it would now crash instead of fail in an assertion.

The _validate_args method is new in this PR. I am at the same time adding a method and then commenting it out... I think it is still useful to leave the _validate method there even if the call is commented out because it at least shows in the code what the types are supposed to be even if that is not actually verified at runtime.

I added it because I wanted to see the full test suite run to confirm my expectations of what the types are. This is because in SymPy it is quite common that unexpected types turn up inside other objects because of a lack of checking. The polys code generally does not do much much checking but surprisingly has the least problems with this just because it is better structured than most of the rest of the codebase: there is always a clear answer for what the types should be.

To be able to use python-flint's types we need to be completely clear what the types of the objects are because C-level types cannot just store arbitrary Python objects and not care whether it is an int or an Integer etc.

[sylee957]

Why does everything need ZZ constructor? Does the strict validation makes it fail?

[oscarbenjamin]

Why does everything need ZZ constructor?

The coefficients of a polynomial over ZZ should be elements of ZZ. The previous code was just being sloppy with types and probably predates there being multiple implementations of ZZ. IF the only implementation of ZZ is int then there is no distinction between 1 and ZZ(1). There are now 3 possible implementations: int, gmpy2.mpz, flint.fmpz. The ZZ constructor will ensure that an object of the correct type is created.

[hanspi42]

This is good for me, if @sylee957 has no further questions, I will merge.

[oscarbenjamin]

Thanks!