Clean up and simplify optimization interface #127
Merged
Conversation
This file contains 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
…int (operator first)
…ptimization and update `fixedpoint` return values
|
I transferred most of the improvements and clean ups from the Manopt PR #105. Most of the things are internal, but also from the user side some things improved:
I have not yet implemented the |
lkdvos
requested changes
Feb 6, 2025
There was a problem hiding this comment.
Overall definitely looks clean and like an improvement!
As I already mentioned in the comments, I think using where clauses in the signature to access certain types is not the most idiomatic, as this is also heavily tied to the internal details of how the types are structured (if we ever change them, we have to go in and change all function signatures as well). A simple scalartype function instead is a lot more portable and future-proof.
Somehow, my brain always thinks about "CTMRG environments" in plural, and the same holds for "MPS environments". This might also just be me, because I can also see why the alternative makes sense. In any case, I agree to decide on one style and keeping that. I guess I would vote plural and you would vote singular, so @leburgel what do you think?
Happy to offer my opinion, but, on what exactly? Are we talking about variable names ( |
Yes please!
I would also assume this is about |
I see. I personally prefer singular, so |
|
One last thing to do before merging: I should finally fix this Zygote The annoying thing is that I can now rarely reproduce this such that it's hard to test possible solutions. Also, it seems that the CI is always running fine without throwing this error. |
|
I found another interesting variant of this problem: before throwing the |
lkdvos
reviewed
Feb 7, 2025
Co-authored-by: Lukas Devos <[email protected]>
This was just running the Heisenberg test, so the arrows should be just the same as always. I'm quite confused by this error. |
|
Using a rrule for |
…Kit.jl into pb-clean-opt-interface
lkdvos
reviewed
Feb 7, 2025
src/algorithms/ctmrg/simultaneous.jl
Outdated
Comment on lines
60
to
62
| condition_number = _condition_number(S) | ||
| _condition_number_pullback(_) = (NoTangent(), ZeroTangent()) | ||
| return condition_number, _condition_number_pullback |
There was a problem hiding this comment.
Suggested change
| condition_number = _condition_number(S) | |
| _condition_number_pullback(_) = (NoTangent(), ZeroTangent()) | |
| return condition_number, _condition_number_pullback | |
| return _condition_number(S), Returns((NoTangent(), ZeroTangent())) |
|
Computing the condition number directly in the projector methods has resulted in a new error: |
|
Can you paste the first couple stacks from the output of Also, you can put things in expandable sections with
some hidden long messages telling you how great you are! 🥇
|
I wasn't able to reproduce the error again because it only comes up occasionally and needs recompilation so one always has to wait for Zygote's TTFX... But I haven't yet encountered the error again with the |
|
Nevermind, here's the full error :-) Thanks for the assistance! |
lkdvos
reviewed
Feb 7, 2025
|
I gotta run for today so I will have to finish this up on Monday. Thanks again for the help! |
This seems to originate here, which leads me to believe that somehow something that was supposed to be an array of tensors got exchanged for a single tensor, since that is what that pullback should be getting. That's where I would start investigating, but I definitely didn't do a very thorough analysis here. (no rush, Monday is obviously fine) |
…Kit.jl into pb-clean-opt-interface
|
I investigated a bit and found the Zygote internals I encountered relatively incomprehensible. The I decided to work around this by scrapping the |
lkdvos
previously approved these changes
Feb 10, 2025
There was a problem hiding this comment.
Looks good to me.
I definitely like not using the buffers better anyways, so that's a good solution to me.
Could you make sure to write a squash commit description that includes the API changes? I noticed for example costfun to cost_function, and I think it would be good if we started paying attention a bit to document changes
lkdvos
reviewed
Feb 10, 2025
lkdvos
approved these changes
Feb 10, 2025
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.
No description provided.