feat(optim): Add adadelta multi_tensor support for complex, with has_complex shortcut
#110631
Conversation
jon-chuang
requested review from
H-Huang,
albanD,
awgu,
d4l3k,
fduwjj,
fegin,
janeyx99,
kiukchung,
kwen2501,
mrshenli,
rohan-varma,
wanchaol,
wz337 and
zhaojuanmao
as code owners
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/110631
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (2 Unrelated Failures)As of commit 0869902 with merge base cf1b494 ( UNSTABLE - The following jobs failed but were likely due to flakiness present on trunk and has been marked as unstable:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
soulitzer
added
the
triaged
lezcano
left a comment
lezcano
left a comment
There was a problem hiding this comment.
Given that the pattern is the same in every optimiser, perhaps we want to factor it out into its own aux function?
|
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
|
@jon-chuang what about #110631 (review)? |
|
Hello, @lezcano I tried to respond at #110635 (comment) As mentioned, we are limited by the functional APIs. Given that it is specific to each optimizer and only used in multi_tensor case, I'm not sure it makes sense to have an aux function as there will be no shared logic nor object-oriented abstraction |
|
Anw, I might have some idea about what you mean, I'll ping you once the PR is up. I can try to have a generic helper function that accepts variadic number of lists |
|
I was thinking of something along the lines of def view_as_real(*params):
assert len(params) > 0
n = len(params[0])
assert all(len(p) == n for p in params)
for i in range(n):
p = params[0][i]
if torch.is_complex(p):
for p_list in params:
p_list[i] = torch.view_as_real(p_list[i])If there can be empty lists, you can filter them out first, of course. |
5 participants
Partial fix: #110606
More on
has_complexshortcut: #110613 (comment)CC: @janeyx99, @mlazos, @lezcano