Skip to content

fix: MultiLoss.__add__ mutates original loss chain - #108

Open
andrewwhitecdw wants to merge 3 commits into
NVlabs:mainfrom
andrewwhitecdw:bugfix/losses-multiloss-add-mutates-original-loss
Open

fix: MultiLoss.__add__ mutates original loss chain#108
andrewwhitecdw wants to merge 3 commits into
NVlabs:mainfrom
andrewwhitecdw:bugfix/losses-multiloss-add-mutates-original-loss

Conversation

@andrewwhitecdw

Copy link
Copy Markdown

Problem

fix: MultiLoss.add mutates original loss chain

Fix

Replace:

    def __add__(self, loss2):
        assert isinstance(loss2, MultiLoss)
        res = cur = copy(self)
        # find the end of the chain
        while cur._loss2 is not None:
            cur = cur._loss2
        cur._loss2 = loss2
        return res

with:

    def __add__(self, loss2):
        assert isinstance(loss2, MultiLoss)
        res = cur = deepcopy(self)
        # find the end of the chain
        while cur._loss2 is not None:
            cur = cur._loss2
        cur._loss2 = loss2
        return res

Files changed

  • dust3r/losses.py

andrewwhitecdw and others added 3 commits July 27, 2026 20:47
Auditor: The diff switches from copy(self) to deepcopy(self) but does not update the corresponding import (currently from copy import copy). After this change, deepcopy would be undefined and raise a NameError, so the diff is incomplete.
Auditor: Using deepcopy is overkill and unsafe for this wrapper chain: it recursively copies the underlying loss objects/modules, changing sharing semantics and potentially duplicating parameters/state. The wrapper chain itself should be shallow-copied instead. The diff also omits the needed import update and includes no regression test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant