Repository navigation
Conversation
ValerianRey
reviewed
Nov 2, 2026
ValerianRey
left a comment
Member
There was a problem hiding this comment.
Hi, thanks for the PR!
Please correct me if I'm wrong, but I think GradDrop's leak param is missing. Apart from that, LGTM.
Also, could you please open a PR with a similar change for the scalarizers?
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #826, as suggested there by @ValerianRey.
Tensors given to aggregators and weightings were stored as plain attributes, so
.to()did not move them. For exampleConstant(torch.tensor([1., 2.])).to(torch.float64)kept its weights infloat32.This registers every such tensor as a buffer instead:
ConstantWeighting.weights(andConstantnow reads its weights from its weighting instead of keeping a second reference in_weights)_pref_vectorofUPGradWeighting,DualProjWeighting,AlignedMTLWeighting,ConFIGandSDMGradWeighting(the aggregators wrapping them get it for free)_initial_weightsofCRMOGMWeightingAttribute access is unchanged (
weights,pref_vector, the setters,repr/str). The stateful state tensors (_w,_lambda,_phi_t, ...) were already buffers, so I left them alone.Open question:
persistent=False. I went with non persistent buffers. These tensors are constructor arguments, likelroralphawhich are not in thestate_dicteither, and making them persistent would add new keys to thestate_dictof any module holding an aggregator, so loading a checkpoint saved with 0.18 usingstrict=Truewould fail with missing keys. The downside is that a checkpoint does not restore them, which is a bit different from the stateful buffers (_w,_lambda, ...) that are persistent. Happy to switch topersistent=Trueif you prefer that.Not included: the scalarizers in
torchjd.scalarization(Constant,STCH,PBI,COSMOS) also keepweights/referenceas plain attributes. I can do the same there in this PR or a separate one if you want.Testing
Added a
test_to_moves_*test for each class above. Each builds the object withfloat32tensors, calls.to(torch.float64), and checks that the tensors are nowfloat64, that they are among the module's buffers, and that they are not in thestate_dict. The 7 new tests fail on main and pass with this change.uv run pytest tests/unit -W error: 3703 passed, 66 skipped, 33 xfailed (CPU)PYTEST_TORCH_DTYPE=float64ruff check,ruff format, pre-commit on changed files: cleanty check: no new diagnostics (only the existing unresolved imports for the optional plot deps I did not install)