Skip to content

Conversation

craymichael
Copy link
Contributor

References to other Issues or PRs

None, afaik

Brief description of what is fixed or changed

Add _print_Exp1 definition to the theanocode printing module. Printed functions containing a sympy.core.numbers.Exp1 instance currently fail to be printed.

Other comments

MWE to reproduce errors:

from sympy.printing.theanocode import TheanoPrinter
import sympy

x1 = sympy.Symbol('x1')
TheanoPrinter()._print(sympy.exp(1))

This gives the following traceback:

Traceback (most recent call last):
  File "/usr/lib/python3.8/code.py", line 90, in runcode
    exec(code, self.locals)
  File "<input>", line 1, in <module>
  File "/usr/lib/python3.8/site-packages/sympy/printing/printer.py", line 289, in _print
    return getattr(self, printmethod)(expr, **kwargs)
  File "/url/lib/python3.8/site-packages/sympy/printing/theanocode.py", line 156, in _print_Basic
    op = mapping[type(expr)]
KeyError: <class 'sympy.core.numbers.Exp1'>

Release Notes

  • printing
    • Fixed a bug where TheanoPrinter fails to print Euler's number e (sympy.core.numbers.Exp1)

Add `_print_Exp1` definition to the `theanocode` printing module. Printed functions containing a `sympy.core.numbers.Exp1` instance will fail to be printed. MWE to reproduce errors:

```python
from sympy.printing.theanocode import TheanoPrinter
import sympy

x1 = sympy.Symbol('x1')
TheanoPrinter()._print(sympy.exp(1))
```

This gives the following traceback:
```
Traceback (most recent call last):
  File "/usr/lib/python3.8/code.py", line 90, in runcode
    exec(code, self.locals)
  File "<input>", line 1, in <module>
  File "/usr/lib/python3.8/site-packages/sympy/printing/printer.py", line 289, in _print
    return getattr(self, printmethod)(expr, **kwargs)
  File "/url/lib/python3.8/site-packages/sympy/printing/theanocode.py", line 156, in _print_Basic
    op = mapping[type(expr)]
KeyError: <class 'sympy.core.numbers.Exp1'>
```
@sympy-bot
Copy link

sympy-bot commented Oct 24, 2020

Hi, I am the SymPy bot (v161). 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
    • Fixed a bug where TheanoPrinter fails to print Euler's number e (sympy.core.numbers.Exp1) (#20335 by @craymichael)

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

Click here to see the pull request description that was parsed.
#### References to other Issues or PRs
None, afaik


#### Brief description of what is fixed or changed
Add `_print_Exp1` definition to the `theanocode` printing module. Printed functions containing a `sympy.core.numbers.Exp1` instance currently fail to be printed.

#### Other comments

MWE to reproduce errors:

```python
from sympy.printing.theanocode import TheanoPrinter
import sympy

x1 = sympy.Symbol('x1')
TheanoPrinter()._print(sympy.exp(1))
```

This gives the following traceback:
```
Traceback (most recent call last):
  File "/usr/lib/python3.8/code.py", line 90, in runcode
    exec(code, self.locals)
  File "<input>", line 1, in <module>
  File "/usr/lib/python3.8/site-packages/sympy/printing/printer.py", line 289, in _print
    return getattr(self, printmethod)(expr, **kwargs)
  File "/url/lib/python3.8/site-packages/sympy/printing/theanocode.py", line 156, in _print_Basic
    op = mapping[type(expr)]
KeyError: <class 'sympy.core.numbers.Exp1'>
```

#### Release Notes

<!-- Write the release notes for this release below. 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
  * Fixed a bug where `TheanoPrinter` fails to print Euler's number _e_ (`sympy.core.numbers.Exp1`)
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

@codecov
Copy link

codecov bot commented Oct 24, 2020

Codecov Report

Merging #20335 (a522d8a) into master (d89bd0d) will decrease coverage by 0.014%.
The diff coverage is 49.098%.

@@              Coverage Diff              @@
##            master    #20335       +/-   ##
=============================================
- Coverage   75.748%   75.733%   -0.015%     
=============================================
  Files          673       673               
  Lines       174050    174393      +343     
  Branches     41099     41198       +99     
=============================================
+ Hits        131840    132074      +234     
- Misses       36493     36601      +108     
- Partials      5717      5718        +1     

@oscarbenjamin
Copy link
Collaborator

Can you add a test that this is working correctly?

@oscarbenjamin
Copy link
Collaborator

Otherwise looks good

@craymichael
Copy link
Contributor Author

Just pushed a test case, let me know if it looks good.

I noticed in the sympy/printing/tests/test_theanocode.py file that test_complexfunctions uses the theano_code instead of theano_code_ function, and test_constantfunctions the theano_function function instead of theano_function_, which may(?) yield unexpected results due to cache. Was this intended?

@oscarbenjamin
Copy link
Collaborator

Was this intended?

No idea. I don't know this part of the codebase so well. You could check the blame and see where the code was added. There might be some discussion there.

Would it not make sense to test the output of the printer rather than (or as well as) the output from theano?

@craymichael
Copy link
Contributor Author

Got you - I will explore the blame.

I do believe the test tries out the printer - the theano_code function (wrapped by theano_code_) calls the printer actually, see theanocode.py.

def theano_code(expr, cache=None, **kwargs):
    ...
    return TheanoPrinter(cache=cache, settings={}).doprint(expr, **kwargs)

I didn't directly call TheanoPrinter simply because the other test cases didn't - either approach I think covers the case. Let me know if I misunderstand.

@craymichael
Copy link
Contributor Author

Just following up on this. Following the other test cases and per my last response, this should sufficiently test the printer for Exp1

@oscargus
Copy link
Contributor

oscargus commented Sep 5, 2021

Closing and reopening to trigger a new test run. Seems OK to me.

@oscargus oscargus closed this Sep 5, 2021
@oscargus oscargus reopened this Sep 5, 2021
@oscargus
Copy link
Contributor

oscargus commented Sep 5, 2021

Looks OK to me! Enabling auto-merge so once the tests pass it will be merged. Thanks for your contribution!

@oscargus oscargus enabled auto-merge September 5, 2021 11:16
@oscargus oscargus merged commit c181470 into sympy:master Sep 5, 2021
@github-actions
Copy link

github-actions bot commented Sep 5, 2021

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)

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants