Model Builder refactoring #119
Conversation
|
@twiecki this is just a starter of final PR, let's not merge it yet as I might find other necessary changes while trying to integrate ModelBuilder with BaseMMM. |
michaelraczycki
changed the title
|
As recently we've came to a conclusion that for variety of projects it will be unsuitable to have Model Builder as a child of pm.Model, this PR will be used for refactoring tasks that will prepare new structure of ModelBuilder. General changes will include: |
| def build_model(self, model_config, data=None): | ||
|
|
||
| def build_model(self, model_instance, model_config, data=None): | ||
| model_instance.model_config = model_config |
There was a problem hiding this comment.
I think we can just keep the model_config and data in the base class (instance self). The init() method should also take care of that.
|
If you rebase from main again, #125 should have fixed CI. |
…xtend_idata to prediction functions, adapted test file
| @@ -37,8 +37,8 @@ class ModelBuilder: | |||
| def __init__( | |||
| self, | |||
| model_config: Dict, | |||
There was a problem hiding this comment.
make model_config also optional and move down.
pymc_experimental/model_builder.py
Outdated
| @@ -233,21 +239,26 @@ def fit( | |||
| self._data_setter(data) | |||
|
|
|||
| with self.model: | |||
| self.idata = pm.sample(**self.sample_config) | |||
| if self.sampler_config is not None: | |||
There was a problem hiding this comment.
in init you can just set sampler_config to {} if it's None.
There was a problem hiding this comment.
true, good catch!
|
@twiecki it seem that after rebasing the original issue with test is solved, but a lot of not-related tests fail both in ubuntu and windows |
|
Hm, I think something went wrong with the rebasing, those errors should be fixed. |
|
Ok, tried to do rebase again, hopefully runs good this time |
* Update API reference listings * improve api page+some fixes * update to pymc theme * add pymc theme to requirements * fix indent in docstring --------- Co-authored-by: Oriol (ZBook) <[email protected]>
| @@ -0,0 +1,5 @@ | |||
| {{ objname | escape | underline }} | |||
There was a problem hiding this comment.
I think we made it worse :P, this hsouldn't show up here. Maybe start with a fresh PR?
There was a problem hiding this comment.
ok, I'll try to just copy the changes to new branch, branched from fresh main and see if it helps
In order to make the model_builder a template class for other model-related pymc classes we need to restructure its constructor. So far it was assuming presence of build_model function in child class which is not always going to be the case.
changes:
build() function call was moved out of the constructor, and now is called from fit function. This change allows inheritance from ModelBuilder by abstract classes like baseMMM