Skip to content

Fix saving/loading ProphetModel #1019

Merged
merged 4 commits into from
Nov 30, 2022
Merged

Conversation

Mr-Geekman
Copy link
Contributor

@Mr-Geekman Mr-Geekman commented Nov 28, 2022

Before submitting (must do checklist)

  • Did you read the contribution guide?
  • Did you update the docs? We use Numpy format for all the methods and classes.
  • Did you write any new necessary tests?
  • Did you update the CHANGELOG?

Proposed Changes

Look #1016.

Closing issues

Closes #1016.

@Mr-Geekman Mr-Geekman self-assigned this Nov 28, 2022
@github-actions
Copy link

github-actions bot commented Nov 28, 2022

@github-actions github-actions bot temporarily deployed to pull request November 28, 2022 18:06 Inactive
Copy link
Collaborator

@alex-hse-repository alex-hse-repository left a comment

Choose a reason for hiding this comment

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

Do we have somewhere test which check that loaded fitted model make the same forecast?


self.__dict__.update(local_state)

if is_fitted:
Copy link
Collaborator

Choose a reason for hiding this comment

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

May be we can move model creation logic to _create_model method, i.e. add optional parameter state

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really think that it is a good idea. Now _create_model doesn't know anything about serialization. Just handles creation of model using hyperparameters and processing additional_seasonalities (I added this method mostly because of them).


self.regressor_columns: Optional[List[str]] = None

def _create_model(self) -> "Prophet":
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add description to the method



def test_setstate_not_fitted():
model_1 = _ProphetAdapter()
Copy link
Collaborator

Choose a reason for hiding this comment

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

May be leave here only one model for simplicity:

initial_state = model.__getstate__() 
new_state = model.__setstate__(model.__getstate__()).__getstate_() 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test and my variation of this test can work even if __setstate__ does nothing. I fixed it.

@Mr-Geekman
Copy link
Contributor Author

Do we have somewhere test which check that loaded fitted model make the same forecast?

Isn't test_save_load enough? Or you want this test exactly for the adapter?

@github-actions github-actions bot temporarily deployed to pull request November 29, 2022 14:11 Inactive
@alex-hse-repository alex-hse-repository merged commit 6ca6f82 into inference-v2 Nov 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants