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

Overwrite environment deepcopy #83

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

lt-brs
Copy link

@lt-brs lt-brs commented Nov 3, 2022

No description provided.

@lt-brs
Copy link
Author

lt-brs commented Nov 3, 2022

Related to #82, overwriting deepcopy enabled to deepcopy a bmb.Model

@tomicapretto
Copy link
Collaborator

Thanks for the PR!

Do you have an example or test we could add?

@lt-brs
Copy link
Author

lt-brs commented Nov 4, 2022

Yes sure. Here is the test I did.

The point is that it should be included in bambinos.bambi (not in formulae) and it needs the version of formulae including the fix.
In bambinos.formulae, the only relevant test would be to ensure deepcopy(env) don't raise error.

import bambi as bmb
import pandas as pd 
from copy import deepcopy

model = bmb.Model('y ~ x', 
          data=pd.DataFrame({'x':[i for i in range(10)], 'y':[2*i+0.1/(1+i) for i in range(10)]})
          )

trace = model.fit(random_seed=0)

clone_model = deepcopy(model)
clone_trace = clone_model.fit(random_seed=0)

assert clone_trace == trace

Thank you for your message

@tomicapretto
Copy link
Collaborator

tomicapretto commented Nov 4, 2022

What happens if you do something like

import bambi as bmb
import numpy as np
import pandas as pd


model = bmb.Model(
    'y ~ np.log(x)', 
    data=pd.DataFrame({'x':[i + 1 for i in range(10)], 'y':[2*i+0.1/(1+i) for i in range(10)]})
)

clone_model = deepcopy(model)
clone_trace = clone_model.fit(random_seed=0)
clone_model.predict(clone_trace, data=pd.DataFrame({"x": [1, 2, 3, 4]})

Does the last predict work?

edit I'm expecting it to fail. Since the environment in the second model is empty (if I understand correctly what's happening with deepcopy) and then it won't be able to find where np.log() comes from.

@lt-brs
Copy link
Author

lt-brs commented Nov 4, 2022

Indeed you are right, the predict fails in this case. Thank you for your help.
I looked at bambinos/bambi#400 and especially the comment of August 24, 2021.
It seems that dill enable to fix the issue. I updated the PR by adding the fix and a dependency to dill

The test you previously mentionned work

@tomicapretto
Copy link
Collaborator

Brilliant! Thanks for taking the work to do ti. I'm triggering CI now but I will be checking with more care tomorrow.

@codecov
Copy link

codecov bot commented Nov 4, 2022

Codecov Report

Merging #83 (716f06c) into master (d7eae85) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #83      +/-   ##
==========================================
+ Coverage   95.54%   95.55%   +0.01%     
==========================================
  Files          30       30              
  Lines        3543     3552       +9     
==========================================
+ Hits         3385     3394       +9     
  Misses        158      158              
Impacted Files Coverage Δ
formulae/environment.py 98.30% <100.00%> (+0.09%) ⬆️
formulae/tests/test_environment.py 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tomicapretto
Copy link
Collaborator

Great to see the CI is passing!

@lt-brs Could you tell me a little more about your use case? I would like to know more about how you're using this. I understand you want to pickle a Bambi model and be able to load it again in the future. But I don't know if you have more use cases in mind. Also, if something you're testing fails, could you let me know, please?

I'm quite confident this change is not breaking anything of the existing functionalities (all tests are good). But I don't have enough confidence to assume the new feature is working as expected yet, simply because I don't have them in mind right now.

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

Successfully merging this pull request may close these issues.

2 participants