-
Notifications
You must be signed in to change notification settings - Fork 150
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
Balanced neural ratio estimation #779
Conversation
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.
Awesome, thanks a lot for this!
One small comment below, but a few additional comments:
- could you add
BNRE
to thetests
here? - if you want, you can add BNRE to the documentation: README, API reference, landing page of the website, and tutorial on all available methods
labels[1::2] = 0.0 | ||
|
||
# Binary cross entropy to learn the likelihood (AALR-specific) | ||
bce = nn.BCELoss()(likelihood, labels) |
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.
Is there a reason to not call super()._loss(theta, x)
?
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.
Yes, I need access to the logits to compute the regularizer and the loss function does not return those.
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.
Makes sense, thank you!
Also, please run |
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.
Great! thanks a lot for adding this!
I added one small comment on the docs, and +1 on adding a functional test for bnre
.
Codecov Report
@@ Coverage Diff @@
## main #779 +/- ##
==========================================
- Coverage 74.51% 74.42% -0.10%
==========================================
Files 77 78 +1
Lines 6118 6143 +25
==========================================
+ Hits 4559 4572 +13
- Misses 1559 1571 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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.
Hi Arnaud,
thanks for the documentation fixes. Could you still add BNRE to the tests and make sure that they pass?
Michael
Hi Michael, Yes sorry for the delay. I have not forgotten, it is my todo list and I will do it asap! |
Great, thanks! No rush from our side! |
Hi, I have now added the tests and updated the specifications to explain what BNRE is. I believe that I have addressed all the comments, tell me if there is something missing! |
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.
Great, thanks! One of the new BNRE tests is failing, I added comments below.
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.
Looks great, thank you for contributing Balanced NRE!
Implement Balanced neural ratio estimation.