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

Remove use of deepcopy #163

Merged
merged 5 commits into from
Apr 10, 2021
Merged

Remove use of deepcopy #163

merged 5 commits into from
Apr 10, 2021

Conversation

SkafteNicki
Copy link
Member

@SkafteNicki SkafteNicki commented Apr 7, 2021

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

What does this PR do?

After profiling some metrics, I saw that that reset is spending a lot of time calling deepcopy. As the recommended method for cloning a tensor is .detach().clone() and it is a bit faster:

master:
tensor state (cpu): 0.8742028713226319+-0.03886386210968593
tensor state (gpu): 2.5115918636322023+-0.21529070194941985
list state (cpu): 2.109953451156616+-0.07316269549459964
list state (gpu): 1.809204387664795+-1.0394736704878353

PR:
tensor state (cpu):0.8499699592590332+-0.01647528474876866
tensor state (gpu):2.2886361598968508+-0.08935389958063863
list state (cpu): 2.0209419250488283+-0.05932843043190113
list state (gpu): 1.547454833984375+-0.407560693521206

calculate for 5 repetitions over 1000 forward calculations (reset is called once for every forward call which is why such a small optimization may matter in the long run).

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

@SkafteNicki SkafteNicki added the enhancement New feature or request label Apr 7, 2021
@codecov
Copy link

codecov bot commented Apr 7, 2021

Codecov Report

Merging #163 (4ac6a02) into master (a17ccb2) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #163     +/-   ##
=========================================
  Coverage   95.99%   95.99%             
=========================================
  Files         168       84     -84     
  Lines        5144     2572   -2572     
=========================================
- Hits         4938     2469   -2469     
+ Misses        206      103    -103     
Flag Coverage Δ
Linux 78.73% <100.00%> (ø)
Windows 78.73% <100.00%> (ø)
cpu 95.99% <100.00%> (ø)
gpu ?
macOS 95.99% <100.00%> (ø)
pytest 95.99% <100.00%> (ø)
python3.6 95.98% <100.00%> (ø)
python3.8 95.99% <100.00%> (ø)
python3.9 95.99% <100.00%> (ø)
torch1.3.1 94.85% <100.00%> (ø)
torch1.4.0 94.98% <100.00%> (ø)
torch1.8.1 95.99% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
torchmetrics/metric.py 95.07% <100.00%> (ø)
...etrics/functional/regression/explained_variance.py
__w/1/s/torchmetrics/regression/ssim.py
...chmetrics/functional/classification/stat_scores.py
...s/torchmetrics/classification/average_precision.py
...ics/functional/classification/average_precision.py
...1/s/torchmetrics/functional/retrieval/precision.py
__w/1/s/torchmetrics/__init__.py
.../s/torchmetrics/classification/hamming_distance.py
__w/1/s/torchmetrics/functional/image_gradients.py
... and 75 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a17ccb2...4ac6a02. Read the comment docs.

Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

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

LGTM !

@SkafteNicki SkafteNicki enabled auto-merge (squash) April 9, 2021 08:55
@SkafteNicki SkafteNicki merged commit ec239e0 into master Apr 10, 2021
@SkafteNicki SkafteNicki deleted the performance branch April 10, 2021 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants