Add setting "strict" to CodePrinter #25913

bjodah · 2023-11-21T10:45:55Z

For the n'th time I'm debugging generated code containing:

# Not supported in Python with numpy:
# Derivative
# Derivative
# Derivative
...

This PR aims to introduce a printer option "strict" ~~so that those of us who prefer an exception upon codegen (fail early) can opt-in to that behavior~~. The default is now True.

References to other Issues or PRs

Brief description of what is fixed or changed

CodePrinter._default_settings got a new entry: strict=True.

Other comments

Subclasses of CodePrinter now "inherits" the entries in CodePrinter._default_settings. This way, in the future, printer options may be introduced in codeprinter.py without touching the respective language's defaults.

Release Notes

printing
- CodePrinter got a new option "strict" (default is generally "True") to raise an exception, instead of generating comments, upon encountering unsupported constructs in a target language.

sympy-bot · 2023-11-21T10:46:03Z

✅

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:

printing
- CodePrinter got a new option "strict" (default is generally "True") to raise an exception, instead of generating comments, upon encountering unsupported constructs in a target language. (#25913 by @bjodah)

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.

For the n'th time I'm debugging generated code containing:
```
# Not supported in Python with numpy:
# Derivative
# Derivative
# Derivative
...
```

This PR aims to introduce a printer option "strict" ~~so that those of us who prefer an exception upon codegen (*fail early*) can opt-in to that behavior~~. The default is now `True`.

#### 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
`CodePrinter._default_settings` got a new entry: `strict=True`.

#### Other comments
Subclasses of `CodePrinter` now "inherits" the entries in `CodePrinter._default_settings`. This way, in the future, printer options may be introduced in `codeprinter.py` without touching the respective language's defaults.

#### 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 -->
* printing
  * CodePrinter got a new option "strict" (default is generally "True") to raise an exception, instead of generating comments, upon encountering unsupported constructs in a target language.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

bjodah · 2023-11-21T10:50:25Z

An option would be to add a branch in CodePrinter._print_not_supported:

diff --git a/sympy/printing/codeprinter.py b/sympy/printing/codeprinter.py
index b717483f8e..2352928b97 100644
--- a/sympy/printing/codeprinter.py
+++ b/sympy/printing/codeprinter.py
@@ -573,6 +573,8 @@ def _print_Mul(self, expr):
             return sign + '*'.join(a_str) + "/(%s)" % '*'.join(b_str)
 
     def _print_not_supported(self, expr):
+        if self._settings.get('strict', False):
+            raise ValueError("Unsupported: %s" % str(type(expr)))
         try:
             self._not_supported.add(expr)
         except TypeError:

perhaps that's cleaner instead of raising much later in doprint?

oscarbenjamin · 2023-11-21T13:04:45Z

This PR aims to introduce a printer option "strict" so that those of us who prefer an exception upon codegen (fail early) can opt-in to that behavior.

Doesn't everyone want this?

Would it make sense for the default to be strict=True?

github-actions · 2023-11-21T13:18:36Z

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 [a00718ba]    | After [39594aac]    |   Ratio | Benchmark (Parameter)                                                |
|----------|----------------------|---------------------|---------|----------------------------------------------------------------------|
| -        | 68.6±0.4ms           | 44.6±0.3ms          |    0.65 | integrate.TimeIntegrationRisch02.time_doit_risch(10)                 |
| +        | 17.2±0.4μs           | 29.7±0.2μs          |    1.72 | integrate.TimeIntegrationRisch03.time_doit(1)                        |
| -        | 5.41±0.03ms          | 2.86±0.05ms         |    0.53 | logic.LogicSuite.time_load_file                                      |
| -        | 71.6±0.4ms           | 28.3±0.1ms          |    0.4  | polys.TimeGCD_GaussInt.time_op(1, 'dense')                           |
| -        | 25.7±0.07ms          | 16.9±0.04ms         |    0.66 | polys.TimeGCD_GaussInt.time_op(1, 'expr')                            |
| -        | 71.4±0.7ms           | 28.6±0.2ms          |    0.4  | polys.TimeGCD_GaussInt.time_op(1, 'sparse')                          |
| -        | 252±0.7ms            | 123±0.5ms           |    0.49 | polys.TimeGCD_GaussInt.time_op(2, 'dense')                           |
| -        | 254±3ms              | 124±0.2ms           |    0.49 | polys.TimeGCD_GaussInt.time_op(2, 'sparse')                          |
| -        | 651±5ms              | 372±1ms             |    0.57 | polys.TimeGCD_GaussInt.time_op(3, 'dense')                           |
| -        | 653±5ms              | 372±3ms             |    0.57 | polys.TimeGCD_GaussInt.time_op(3, 'sparse')                          |
| -        | 501±3μs              | 284±3μs             |    0.57 | polys.TimeGCD_LinearDenseQuadraticGCD.time_op(1, 'dense')            |
| -        | 2.05±0.07ms          | 1.04±0ms            |    0.51 | polys.TimeGCD_LinearDenseQuadraticGCD.time_op(2, 'dense')            |
| -        | 5.80±0.07ms          | 3.02±0.02ms         |    0.52 | polys.TimeGCD_LinearDenseQuadraticGCD.time_op(3, 'dense')            |
| -        | 452±1μs              | 226±2μs             |    0.5  | polys.TimeGCD_QuadraticNonMonicGCD.time_op(1, 'dense')               |
| -        | 1.45±0.01ms          | 668±7μs             |    0.46 | polys.TimeGCD_QuadraticNonMonicGCD.time_op(2, 'dense')               |
| -        | 4.84±0.08ms          | 1.61±0.02ms         |    0.33 | polys.TimeGCD_QuadraticNonMonicGCD.time_op(3, 'dense')               |
| -        | 377±2μs              | 200±1μs             |    0.53 | polys.TimeGCD_SparseGCDHighDegree.time_op(1, 'dense')                |
| -        | 2.42±0ms             | 1.19±0ms            |    0.49 | polys.TimeGCD_SparseGCDHighDegree.time_op(3, 'dense')                |
| -        | 10.1±0.06ms          | 4.34±0.02ms         |    0.43 | polys.TimeGCD_SparseGCDHighDegree.time_op(5, 'dense')                |
| -        | 361±0.8μs            | 164±1μs             |    0.46 | polys.TimeGCD_SparseNonMonicQuadratic.time_op(1, 'dense')            |
| -        | 2.44±0.01ms          | 874±5μs             |    0.36 | polys.TimeGCD_SparseNonMonicQuadratic.time_op(3, 'dense')            |
| -        | 9.38±0.04ms          | 2.54±0.01ms         |    0.27 | polys.TimeGCD_SparseNonMonicQuadratic.time_op(5, 'dense')            |
| -        | 1.03±0.01ms          | 423±5μs             |    0.41 | polys.TimePREM_LinearDenseQuadraticGCD.time_op(3, 'dense')           |
| -        | 1.71±0.01ms          | 501±2μs             |    0.29 | polys.TimePREM_LinearDenseQuadraticGCD.time_op(3, 'sparse')          |
| -        | 5.81±0.05ms          | 1.76±0.01ms         |    0.3  | polys.TimePREM_LinearDenseQuadraticGCD.time_op(5, 'dense')           |
| -        | 8.46±0.06ms          | 1.50±0.01ms         |    0.18 | polys.TimePREM_LinearDenseQuadraticGCD.time_op(5, 'sparse')          |
| -        | 292±1μs              | 63.4±0.8μs          |    0.22 | polys.TimePREM_QuadraticNonMonicGCD.time_op(1, 'sparse')             |
| -        | 3.45±0.02ms          | 388±2μs             |    0.11 | polys.TimePREM_QuadraticNonMonicGCD.time_op(3, 'dense')              |
| -        | 3.92±0.04ms          | 275±2μs             |    0.07 | polys.TimePREM_QuadraticNonMonicGCD.time_op(3, 'sparse')             |
| -        | 6.83±0.04ms          | 1.25±0.01ms         |    0.18 | polys.TimePREM_QuadraticNonMonicGCD.time_op(5, 'dense')              |
| -        | 8.59±0.05ms          | 842±0.9μs           |    0.1  | polys.TimePREM_QuadraticNonMonicGCD.time_op(5, 'sparse')             |
| -        | 5.13±0.04ms          | 2.98±0.01ms         |    0.58 | polys.TimeSUBRESULTANTS_LinearDenseQuadraticGCD.time_op(2, 'sparse') |
| -        | 11.9±0.04ms          | 6.43±0.07ms         |    0.54 | polys.TimeSUBRESULTANTS_LinearDenseQuadraticGCD.time_op(3, 'dense')  |
| -        | 22.1±0.1ms           | 9.15±0.02ms         |    0.41 | polys.TimeSUBRESULTANTS_LinearDenseQuadraticGCD.time_op(3, 'sparse') |
| -        | 5.29±0.05ms          | 885±10μs            |    0.17 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(1, 'sparse')    |
| -        | 12.8±0.06ms          | 7.05±0.03ms         |    0.55 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(2, 'sparse')    |
| -        | 99.6±0.2ms           | 25.5±0.1ms          |    0.26 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(3, 'dense')     |
| -        | 166±0.9ms            | 55.2±0.5ms          |    0.33 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(3, 'sparse')    |
| -        | 172±0.9μs            | 114±2μs             |    0.67 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(1, 'dense')      |
| -        | 360±3μs              | 213±1μs             |    0.59 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(1, 'sparse')     |
| -        | 4.25±0.03ms          | 837±3μs             |    0.2  | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(3, 'dense')      |
| -        | 5.21±0.01ms          | 376±3μs             |    0.07 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(3, 'sparse')     |
| -        | 19.5±0.1ms           | 2.76±0.02ms         |    0.14 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(5, 'dense')      |
| -        | 22.4±0.1ms           | 626±4μs             |    0.03 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(5, 'sparse')     |
| -        | 486±3μs              | 132±0.7μs           |    0.27 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(1, 'sparse') |
| -        | 4.65±0.03ms          | 609±4μs             |    0.13 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(3, 'dense')  |
| -        | 5.23±0.01ms          | 134±1μs             |    0.03 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(3, 'sparse') |
| -        | 12.6±0.05ms          | 1.27±0.01ms         |    0.1  | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(5, 'dense')  |
| -        | 13.5±0.04ms          | 138±2μs             |    0.01 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(5, 'sparse') |
| -        | 140±0.7μs            | 72.9±0.4μs          |    0.52 | solve.TimeMatrixOperations.time_rref(3, 0)                           |
| -        | 267±0.9μs            | 86.9±0.8μs          |    0.33 | solve.TimeMatrixOperations.time_rref(4, 0)                           |
| -        | 24.3±0.2ms           | 10.2±0.03ms         |    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).

bjodah · 2023-11-21T16:48:56Z

I guess you are right. Unless there are those who do some manual postprocessing of the generated code? But I suppose those are few, and we could include some information in the exception message that there is a printer option called "strict" that can be set to False in order to generate partially complete code?

bjodah · 2023-11-21T17:17:46Z

I pushed 82f80c0 to change the default of strict to True. Or rather, as highlighted by this test:

sympy/sympy/printing/tests/test_fortran.py

Line 43 in 69d3af7

assert fcode(expr2, human=False)[2].lstrip() == "exp(re(x) + re(y + z))"

The default shouldprobably only be True when human == True. So the default is actually None and then the value of human is used for strict if not given explicitly.

bjodah · 2023-11-22T14:52:21Z

So changing the default to strict=True was a bit more invasive than anticipated. But now tests pass at least.

oscarbenjamin · 2023-11-22T15:37:48Z

sympy/printing/tests/test_codeprinter.py

-    with raises(NotImplementedError):
+    with raises(ValueError):
        c.doprint(ImmutableSparseMatrix(2, 2, {}))
-    with raises(NotImplementedError):
+    with raises(ValueError):
        c.doprint(MutableSparseMatrix(2, 2, {}))


Would NotImplementedError be more appropriate here?

Or perhaps actually a special exception class.

I agree. I added a new Exception class.

oscarbenjamin · 2023-11-27T23:52:29Z

Looks good!

This is a new option introduced in 1.13.0 which defaults to `True`. Setting it to `False` makes CodePrinter behave as before. See: sympy/sympy/pull/25913 Fix ANNarchy#27

Add setting strict=False to CodePrinter

aeb0680

bjodah added 2 commits November 21, 2023 13:30

CodePrinter strict option: raise in _print_not_supported

cff64c1

It was not even possible to override _print_not_supported

111cc83

CodePrinter strict option: change default of strict to True

82f80c0

bjodah force-pushed the add-strict-printer-setting branch from 18ecb42 to 82f80c0 Compare November 21, 2023 17:15

Code-style fixes

a1c7474

bjodah force-pushed the add-strict-printer-setting branch from 4fbd6d8 to a1c7474 Compare November 21, 2023 17:22

bjodah changed the title ~~Add setting strict=False to CodePrinter~~ Add setting "strict" to CodePrinter Nov 21, 2023

bjodah added 2 commits November 22, 2023 11:36

Fix test failures

0eff8eb

Update autowrap to set strict=False when human=False

db58bc9

oscarbenjamin reviewed Nov 22, 2023

View reviewed changes

oscarbenjamin added the codegen label Nov 22, 2023

Introduce PrintMethodNotImplementedError

36d2e6e

bjodah force-pushed the add-strict-printer-setting branch from b18d789 to 36d2e6e Compare November 26, 2023 20:44

oscarbenjamin merged commit b93a982 into sympy:master Nov 27, 2023

bjodah mentioned this pull request Mar 23, 2024

Raise ValueError if Symbol does not print to valid C variable name. #26360

Merged

bjodah deleted the add-strict-printer-setting branch March 23, 2024 23:39

bjodah mentioned this pull request Jun 4, 2024

Regression: lambdify of Derivative instance now throws PrintMethodNotImplementedError #26663

Open

oscarbenjamin mentioned this pull request Jun 11, 2024

lambdify is no longer able to deal with sympy.vector.BaseScalar #26690

Closed

penguinpee mentioned this pull request Jul 21, 2024

Use strict=False for CodePrinter ANNarchy/ANNarchy#28

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Add setting "strict" to CodePrinter #25913

Add setting "strict" to CodePrinter #25913

Uh oh!

bjodah commented Nov 21, 2023 •

edited

Loading

Uh oh!

sympy-bot commented Nov 21, 2023 •

edited

Loading

Uh oh!

bjodah commented Nov 21, 2023

Uh oh!

oscarbenjamin commented Nov 21, 2023

Uh oh!

github-actions bot commented Nov 21, 2023 •

edited

Loading

Uh oh!

bjodah commented Nov 21, 2023

Uh oh!

bjodah commented Nov 21, 2023 •

edited

Loading

Uh oh!

bjodah commented Nov 22, 2023

Uh oh!

oscarbenjamin Nov 22, 2023

Uh oh!

oscarbenjamin Nov 22, 2023

Uh oh!

bjodah Nov 27, 2023

Uh oh!

oscarbenjamin commented Nov 27, 2023

Uh oh!

Uh oh!

Uh oh!

Add setting "strict" to CodePrinter #25913

Add setting "strict" to CodePrinter #25913

Uh oh!

Conversation

bjodah commented Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

Uh oh!

sympy-bot commented Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjodah commented Nov 21, 2023

Uh oh!

oscarbenjamin commented Nov 21, 2023

Uh oh!

github-actions bot commented Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjodah commented Nov 21, 2023

Uh oh!

bjodah commented Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjodah commented Nov 22, 2023

Uh oh!

oscarbenjamin Nov 22, 2023

Choose a reason for hiding this comment

Uh oh!

oscarbenjamin Nov 22, 2023

Choose a reason for hiding this comment

Uh oh!

bjodah Nov 27, 2023

Choose a reason for hiding this comment

Uh oh!

oscarbenjamin commented Nov 27, 2023

Uh oh!

Uh oh!

bjodah commented Nov 21, 2023 •

edited

Loading

sympy-bot commented Nov 21, 2023 •

edited

Loading

github-actions bot commented Nov 21, 2023 •

edited

Loading

bjodah commented Nov 21, 2023 •

edited

Loading