Skip to content

FIX: to_dict with nn models #949

Merged
merged 8 commits into from
Sep 23, 2022
Merged

FIX: to_dict with nn models #949

merged 8 commits into from
Sep 23, 2022

Conversation

martins0n
Copy link
Contributor

@martins0n martins0n commented Sep 19, 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

Closing issues

@github-actions
Copy link

github-actions bot commented Sep 19, 2022

🚀 Deployed on https://deploy-preview-949--etna-docs.netlify.app

@github-actions github-actions bot temporarily deployed to pull request September 19, 2022 15:43 Inactive
@codecov-commenter
Copy link

codecov-commenter commented Sep 19, 2022

Codecov Report

Merging #949 (c3c641d) into master (eea7498) will increase coverage by 0.07%.
The diff coverage is 98.07%.

@@            Coverage Diff             @@
##           master     #949      +/-   ##
==========================================
+ Coverage   85.31%   85.38%   +0.07%     
==========================================
  Files         147      148       +1     
  Lines        7890     7936      +46     
==========================================
+ Hits         6731     6776      +45     
- Misses       1159     1160       +1     
Impacted Files Coverage Δ
etna/core/utils.py 96.15% <96.15%> (ø)
etna/core/mixins.py 97.22% <100.00%> (+0.07%) ⬆️
etna/libs/pytorch_lightning/callbacks.py 100.00% <100.00%> (ø)
etna/models/nn/mlp.py 100.00% <100.00%> (ø)
etna/models/nn/rnn.py 100.00% <100.00%> (ø)
etna/__init__.py

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

@martins0n martins0n self-assigned this Sep 19, 2022
@github-actions github-actions bot temporarily deployed to pull request September 19, 2022 16:20 Inactive
@@ -80,6 +93,22 @@ def to_dict(self):
params["_target_"] = BaseMixin._get_target_from_class(self)
return params

@staticmethod
def _unsafe_to_dict(value: Any) -> Dict[str, Any]:
"""Collect all information about etna object in dict."""
Copy link
Contributor

Choose a reason for hiding this comment

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

It isn't only about etna object, we use it for callbacks from pytorch_lightning.

@github-actions github-actions bot temporarily deployed to pull request September 20, 2022 15:44 Inactive
@github-actions github-actions bot temporarily deployed to pull request September 22, 2022 09:48 Inactive

self._init_params = {}
args_dict = dict(
zip([arg for arg, param in init_args.items() if param.kind == param.POSITIONAL_OR_KEYWORD], deepcopy_args)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we meet problems with other kinds? E.g. kwargs parameters: docs.

Copy link
Contributor Author

@martins0n martins0n Sep 22, 2022

Choose a reason for hiding this comment

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

They will be in kwargs

Copy link
Contributor

Choose a reason for hiding this comment

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

Will it work with POSITIONAL_ONLY somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should write in documentation about the limitations of this approach.

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've added comment

@github-actions github-actions bot temporarily deployed to pull request September 22, 2022 21:08 Inactive
@martins0n martins0n merged commit 76b8081 into master Sep 23, 2022
@martins0n martins0n deleted the fix/nn-params-dump branch September 23, 2022 10:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants