-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Set theano config in model context #2103
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
Conversation
Nice, this one was bothersome for quite some time, thanks for taking this on. I like the approach. |
I strongly support this PR. That's the thing that always annoys |
pymc3/model.py
Outdated
|
||
def __enter__(self): | ||
# __enter__ is called by InitContextMeta before __init__ | ||
if hasattr(self, '_theano_config'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No check on exit. Do you really need it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If config is set in __new__
this code can be omitted. Attribute will be already set
pymc3/model.py
Outdated
@@ -434,6 +439,23 @@ def __init__(self, name='', model=None): | |||
self.potentials = treelist() | |||
self.missing_values = treelist() | |||
|
|||
if theano_config is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point having this check. I would also force testvalue in dict
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean adding the testval option even when a config dict is specified? I guess we could add it if it is not already in there, I'm not sure about this though.
pymc3/model.py
Outdated
self._theano_config = theano.configparser.change_flags(**theano_config) | ||
# We need to enter the theano config, since variables could be | ||
# created in the init of subclasses. | ||
self._theano_config.__enter__() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You do not need it. Init is done in modelcontext
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think I do. The problem is that subclasses of Model
are allowed to add variables in __init__
, so __enter__
is called before Model.__init__
using some metaclass magic. But maybe it would be better to move this to the metaclass instead of doing it in init.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm I see the problem now. Yes, It can be done in __new__
(here), that's the best place for setting config.
|
@ferrine The config is set in |
Let's wait for tests |
602a299
to
81e7bcd
Compare
|
@twiecki The problem were the tests generated by yield. The generators were executed before the fixture that sets |
Sounds good. |
Thanks @aseyboldt, great to finally have this issue resolved. |
I am trying to set |
This should fix #566. I'm not sure this is the best way to do this, it might be better to include the theano context in
Context
, but this gets a bit complicated because we need to pass arguments though metaclasses and multiple inheritance.I removed
sum_div_dimshuffle_bug
. Couldn't even find a reference to this in the theano config. I guess this was removed at some point?I set
compute_test_value
to 'ignore' in some functions about the hessian, because I had some trouble with those (see Theano/Theano#5907).