Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove theanof.set_theano_conf and instead use the config context #4329

Conversation

michaelosthege
Copy link
Member

A pm.Model can take a theano_config kwarg and defaults it to {"compute_test_value": "raise"}.
The Theano config was changed using theanof.set_theano_config, which accessed private Theano APIs that were recently refactored on Theano-PyMC.

This PR removes that theanof.set_theano_config entirely and instead assigns the change_flags context as an attribute. The __enter__ and __exit__ methods are called accordingly.

The interesting thing to see in the CI will be if this has any side-effects.

  • what are the (breaking) changes: Removal of function, mentioned in RELEASE-NOTES
  • important background, or details about the implementation
  • test/docstring coverage: Code was removed. It's quite at the core of pm.Model, so if something is wrong the CI will go 🎆

@codecov
Copy link

codecov bot commented Dec 12, 2020

Codecov Report

Merging #4329 (b365b6b) into master (6f15cbb) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4329      +/-   ##
==========================================
+ Coverage   87.66%   87.67%   +0.01%     
==========================================
  Files          88       88              
  Lines       14264    14245      -19     
==========================================
- Hits        12504    12489      -15     
+ Misses       1760     1756       -4     
Impacted Files Coverage Δ
pymc3/model.py 88.90% <100.00%> (+0.02%) ⬆️
pymc3/theanof.py 90.47% <100.00%> (+0.86%) ⬆️

Copy link
Contributor

@brandonwillard brandonwillard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. My only recommendation is to squash the two commits.

@michaelosthege michaelosthege merged commit 70fdcf9 into pymc-devs:master Dec 13, 2020
@michaelosthege michaelosthege deleted the dont-touch-theano-config-privates branch December 13, 2020 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants