Skip to content

FIX: deepcopy for fitted deepmodel #735

Merged
merged 5 commits into from
Jun 7, 2022

Conversation

martins0n
Copy link
Contributor

@martins0n martins0n commented Jun 3, 2022

IMPORTANT: Please do not create a Pull Request without creating an issue first.

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?

Type of Change

  • Examples / docs / tutorials / contributors update
  • Bug fix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves an existing feature)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Proposed Changes

I propose deepcopy behaviour changes for deep models: we dont copy model and trainer attributes

Reason:

  • torch.Tensors with attached gradients are not supporting deepcopy protocol. (Only Tensors created explicitly by the user "
    "(graph leaves) support the deepcopy protocol at the moment")
  • we use deepcopy after fit for empirical prediction intervals

Related Issue

Closing issues

@github-actions
Copy link

github-actions bot commented Jun 3, 2022

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

@github-actions github-actions bot temporarily deployed to pull request June 3, 2022 19:08 Inactive
@codecov-commenter
Copy link

codecov-commenter commented Jun 3, 2022

Codecov Report

Merging #735 (16d4721) into master (418ed17) will decrease coverage by 0.10%.
The diff coverage is 43.75%.

@@            Coverage Diff             @@
##           master     #735      +/-   ##
==========================================
- Coverage   83.67%   83.57%   -0.11%     
==========================================
  Files         120      121       +1     
  Lines        6629     6643      +14     
==========================================
+ Hits         5547     5552       +5     
- Misses       1082     1091       +9     
Impacted Files Coverage Δ
etna/models/nn/utils.py 25.00% <25.00%> (ø)
etna/models/nn/deepar.py 100.00% <100.00%> (ø)
etna/models/nn/tft.py 100.00% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@github-actions github-actions bot temporarily deployed to pull request June 6, 2022 08:09 Inactive
@github-actions github-actions bot temporarily deployed to pull request June 6, 2022 21:09 Inactive
@martins0n martins0n self-assigned this Jun 7, 2022
@martins0n martins0n requested a review from Mr-Geekman June 7, 2022 07:46
@martins0n martins0n requested a review from Mr-Geekman June 7, 2022 13:27
@github-actions github-actions bot temporarily deployed to pull request June 7, 2022 13:30 Inactive
"""Mixin for `__deepcopy__` behaviour overriding."""

def __deepcopy__(self, memo):
"""Drop `model` and `trainer` attributes while deepcopy."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Same about variable names.



class _DeepCopyMixin:
"""Mixin for `__deepcopy__` behaviour overriding."""
Copy link
Contributor

@Mr-Geekman Mr-Geekman Jun 7, 2022

Choose a reason for hiding this comment

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

Variable names should be surrounded by two surrounding characters instead of one.

Otherwise in ReST it will be italic instead of monotype.

Copy link
Contributor

Choose a reason for hiding this comment

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

Everything else is fine.

@github-actions github-actions bot temporarily deployed to pull request June 7, 2022 14:06 Inactive
@martins0n martins0n merged commit aeac093 into master Jun 7, 2022
@martins0n martins0n deleted the fix/nn-models-deepcopy-after-fit-issue branch June 7, 2022 15:06
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