Repository navigation
QuadprogProjector does not validate norm_eps and reg_eps at construction #810
Copy link
Copy link
Closed
Labels
good first issueIssue that should be easy to solve for new contributorsIssue that should be easy to solve for new contributors
Description
The norm_eps and reg_eps setters of QuadprogProjector raise a ValueError on negative values, but __init__ assigns self._norm_eps and self._reg_eps directly, so the check is skipped at construction:
TorchJD/src/torchjd/_linalg/_dual_cone.py
Lines 74 to 102 in 9ba45e3
| def __init__( | |
| self, | |
| *, | |
| norm_eps: float = 0.0001, | |
| reg_eps: float = 0.0001, | |
| ) -> None: | |
| super().__init__() | |
| self._norm_eps = norm_eps | |
| self._reg_eps = reg_eps | |
| @property | |
| def norm_eps(self) -> float: | |
| return self._norm_eps | |
| @norm_eps.setter | |
| def norm_eps(self, value: float) -> None: | |
| if value < 0.0: | |
| raise ValueError(f"norm_eps must be non-negative, but got {value}.") | |
| self._norm_eps = value | |
| @property | |
| def reg_eps(self) -> float: | |
| return self._reg_eps | |
| @reg_eps.setter | |
| def reg_eps(self, value: float) -> None: | |
| if value < 0.0: | |
| raise ValueError(f"reg_eps must be non-negative, but got {value}.") | |
| self._reg_eps = value |
>>> from torchjd.linalg import QuadprogProjector
>>> p = QuadprogProjector(norm_eps=-1.0, reg_eps=-5.0) # accepted
>>> p.norm_eps = -1.0
ValueError: norm_eps must be non-negative, but got -1.0.A negative reg_eps is subtracted from the diagonal of the Gramian, which can make the quadratic program non-convex. The user then gets a "Failed to solve the quadratic programming problem." error much later, instead of a clear error when creating the projector. The aggregators (e.g. CAGradWeighting) already go through their setters in __init__.
Suggested fix
super().__init__()
self.norm_eps = norm_eps
self.reg_eps = reg_epsChecklist
- Use the setters in
QuadprogProjector.__init__. - Add a test in
tests/unit/linalg/test_dual_cone.pychecking thatQuadprogProjector(norm_eps=-1.0)andQuadprogProjector(reg_eps=-1.0)raise aValueError.
See CONTRIBUTING.md to set up the development environment.
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
good first issueIssue that should be easy to solve for new contributorsIssue that should be easy to solve for new contributors
Activity