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

Clear list states (i.e. delete their contents), not reassign the default [] #2493

Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
11df0eb
Clear (i.e. delete) list state items, not simply overwrite. Previous …
Apr 6, 2024
1fa7077
Added test to check list states elements are deleted (even when refer…
Apr 7, 2024
4b5c099
Updated documentation - highlighted reset clears list states, and tha…
Apr 9, 2024
8bf151b
Add missing method (sphinx) role
dominicgkerr Apr 9, 2024
64fd4d2
Merge branch 'master' into bugfix/2492-clear-list-states-not-reassign
Borda Apr 10, 2024
b991b3b
Merge branch 'master' into bugfix/2492-clear-list-states-not-reassign
mergify[bot] Apr 11, 2024
82f808b
changelog
SkafteNicki Apr 12, 2024
65b02fa
Remove failing testcode example (fixing introduces too much complexity)
Apr 12, 2024
b9dcc8b
Merge branch 'master' into bugfix/2492-clear-list-states-not-reassign
mergify[bot] Apr 13, 2024
5565524
Merge branch 'bugfix/2492-clear-list-states-not-reassign' of github.c…
Apr 13, 2024
6241a6b
Linting - Line break docstring
Apr 13, 2024
5758977
copy internal states in forward
SkafteNicki Apr 13, 2024
c9d2a86
Detach Tensor | list[Tensor] state values before copying.
Apr 13, 2024
e1872de
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Apr 13, 2024
afdd4c5
Use 'typing' type hints
Apr 13, 2024
76cc0a1
Merge remote-tracking branch 'origin/bugfix/2492-clear-list-states-no…
Apr 13, 2024
ef27215
DO not clone (when caching) Tensor states, but retain references to a…
Apr 13, 2024
e104587
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Apr 13, 2024
21c7970
Revert "DO not clone (when caching) Tensor states, but retain referen…
Apr 13, 2024
51975a8
Added mypy type-hinting requirement/recommendation
Apr 13, 2024
5954d02
Moved update from test checking .__init__ memory leakage. Added test …
Apr 13, 2024
cd11bb0
Merge branch 'master' into bugfix/2492-clear-list-states-not-reassign
mergify[bot] Apr 14, 2024
dc14e5e
Merge branch 'master' into bugfix/2492-clear-list-states-not-reassign
SkafteNicki Apr 15, 2024
925a3b1
Merge branch 'master' into bugfix/2492-clear-list-states-not-reassign
Borda Apr 15, 2024
3f013cf
Fix unused loop control variable for pre-commit
stancld Apr 16, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Fixed axis names with Precision-Recall curve ([#2462](https://github.com/Lightning-AI/torchmetrics/pull/2462))


- Fixed memory leak in metrics using list states ([#2492](https://github.com/Lightning-AI/torchmetrics/pull/2492))


## [1.3.2] - 2024-03-18

### Fixed
Expand Down
11 changes: 11 additions & 0 deletions docs/source/pages/implement.rst
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,17 @@ A few important things to note for this example:
``dim_zero_cat`` helper function which will standardize the list states to be a single concatenate tensor regardless
of the mode.

* Calling the ``reset`` method will clear the list state, deleting any values inserted into it. For this reason, care
must be taken when referencing list states. If you require the values after your metric is reset, you must first
copy the attribute to another object:

.. testcode::

x = metric.list_state # referenced (and deleted by reset)

from deepcopy import copy
y = copy(metric.list_state) # copied (and unchanged by reset)

*****************
Metric attributes
*****************
Expand Down
7 changes: 6 additions & 1 deletion src/torchmetrics/metric.py
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,11 @@ def add_state(
When passing a custom function to ``dist_reduce_fx``, expect the synchronized metric state to follow
the format discussed in the above note.

Note:
The values inserted into a list state are deleted whenever :meth:`~Metric.reset` is called. This allows
device memory to be automatically reallocated, but may produce unexpected effects when referencing list
states. To retain such values after :meth:`~Metric.reset` is called, you must first copy them to another object

Raises:
ValueError:
If ``default`` is not a ``tensor`` or an ``empty list``.
Expand Down Expand Up @@ -681,7 +686,7 @@ def reset(self) -> None:
if isinstance(default, Tensor):
setattr(self, attr, default.detach().clone().to(current_val.device))
else:
setattr(self, attr, [])
getattr(self, attr).clear() # delete/free list items

# reset internal states
self._cache = None
Expand Down
8 changes: 7 additions & 1 deletion tests/unittests/bases/test_metric.py
Original file line number Diff line number Diff line change
Expand Up @@ -124,11 +124,17 @@ class B(DummyListMetric):
metric = B()
assert isinstance(metric.x, list)
assert len(metric.x) == 0
metric.x = tensor(5)
metric.x = [tensor(5)]
metric.reset()
assert isinstance(metric.x, list)
assert len(metric.x) == 0

metric = B()
metric.x = [1, 2, 3]
reference = metric.x # prevents garbage collection
metric.reset()
assert len(reference) == 0 # check list state is freed


def test_reset_compute():
"""Test that `reset`+`compute` methods works as expected."""
Expand Down
Loading