-
Notifications
You must be signed in to change notification settings - Fork 1
Moving to L0 package for HardConcrete regularization implementation #83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
All existing tests (including a specific regularization test) passed though maybe we want to add some more comprehensive ones. Any specific gate behavior it is crucial to capture @baogorek ? |
baogorek
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good. I was just a bit confused because I thought we had moved to a claude-code refactored version of the code at https://github.com/AMLab-Amsterdam/L0_regularization. Now I see that it is a fork of that codebase, but that we have brought in our own HardConcrete class, so by importing that class, we're bypassing the functionality from the external github package.
I think that's fine for now, as this PR still contributes modularity to this repo. I still have doubts about our L0 implementation (which I vibe-coded), so I will spend some time there. FYI @MaxGhenis
|
I don't follow Ben - isn't this moving from your version of L0 to the version I vibe-coded in the L0 package? |
|
@MaxGhenis , what led me to believe that was noticing that the interface to the procedure is exactly identical in this PR: Then, here's what Claude Code said about it: Yeah, I just checked. There's no HardConcrete class in the AMLab-Amsterdam repo. I'm guessing Claude Code knew about it when it was doing the cleanup? Not surprisingly (since it's parent LLM made it), Claude loves our HardConcrete class and thinks we should keep using it. |

Fix #84