Skip to content

Conversation

craymichael
Copy link
Contributor

References to other Issues or PRs

Amends #15885

Brief description of what is fixed or changed

test_complexfunctions uses theano_code instead of theano_code_ and test_constantfunctions uses theano_function instead of theano_function_, which may yield unexpected results due to how the wrapper handles cache. This PR uses the correct definitions.

Other comments

--

Release Notes

  • printing
    • Amended a test case of TheanoPrinter constant and complex functions

@sympy-bot
Copy link

sympy-bot commented Nov 27, 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
    • Amended a test case of TheanoPrinter constant and complex functions (#20500 by @craymichael)

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

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
Amends https://github.com/sympy/sympy/pull/15885


#### Brief description of what is fixed or changed
`test_complexfunctions` uses `theano_code` instead of `theano_code_` and `test_constantfunctions` uses `theano_function` instead of `theano_function_`, which may yield unexpected results due to how the wrapper handles cache. This PR uses the correct definitions.

#### Other comments
--

#### Release Notes

<!-- BEGIN RELEASE NOTES -->
* printing
  * Amended a test case of `TheanoPrinter` constant and complex functions
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

@codecov
Copy link

codecov bot commented Nov 28, 2020

Codecov Report

Merging #20500 (aa18980) into master (524467d) will decrease coverage by 0.034%.
The diff coverage is n/a.

❗ Current head aa18980 differs from pull request most recent head 34074e2. Consider uploading reports for the commit 34074e2 to get more accurate results

@@              Coverage Diff              @@
##            master    #20500       +/-   ##
=============================================
- Coverage   75.781%   75.747%   -0.035%     
=============================================
  Files          673       673               
  Lines       174510    174391      -119     
  Branches     41202     41198        -4     
=============================================
- Hits        132247    132097      -150     
- Misses       36546     36584       +38     
+ Partials      5717      5710        -7     

@oscarbenjamin
Copy link
Collaborator

This looks good.

I'm going to close and reopen to restart the tests.

@craymichael
Copy link
Contributor Author

I just took care of the merge conflict. This should still be good to merge once the tests finish

@oscarbenjamin
Copy link
Collaborator

Sorry I forgot abut this. Are you aware that in the mean time sympy has now moved to supporting aesara? I'm not sure if that affects this patch or not.

@oscarbenjamin
Copy link
Collaborator

@oscarbenjamin
Copy link
Collaborator

CC @brandonwillard

@craymichael
Copy link
Contributor Author

No worries - and I have not, thanks for pointing that out. The move makes sense.

I am not familiar with aesara beyond what Theano was, but the fixes in this PR simply ensure that the theano printer uses the correct wrappers in test functions (just making sure that the cache for theano_code and theano_function is empty by default).

@oscarbenjamin
Copy link
Collaborator

Okay, thanks. I'm reassured to hear from someone who is (I assume) using sympy with theano.

@oscarbenjamin oscarbenjamin enabled auto-merge April 26, 2021 01:03
@oscarbenjamin oscarbenjamin merged commit 9c5bf81 into sympy:master Apr 26, 2021
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.

4 participants