-
Notifications
You must be signed in to change notification settings - Fork 158
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
refactor mixed density estimation #1203
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1203 +/- ##
==========================================
- Coverage 84.55% 75.97% -8.59%
==========================================
Files 96 97 +1
Lines 7603 7668 +65
==========================================
- Hits 6429 5826 -603
- Misses 1174 1842 +668
Flags with carried forward coverage won't be shown. Click here to find out more.
|
3f00930
to
00dbd51
Compare
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.
Thanks a ton, this is awesome!
For now, this can only handle 1D discrete dimensions, right?
In the long run, the ultimate solution would be to have an Autoregressive
abstraction which just concatenates a list of Estimator
s (can be DensityEstimator
or MassEstimator
) in an autoregressive way. No need to do this now ofc.
@@ -101,6 +101,8 @@ def log_prob(self, input: Tensor, condition: Tensor) -> Tensor: | |||
Args: | |||
input: Inputs to evaluate the log probability on. Of shape | |||
`(sample_dim, batch_dim, *event_shape)`. | |||
# TODO: the docstring is not correct here. in the code it seems we | |||
do not have a sample_dim for the condition. |
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.
we do not have a sample_dim
for the condition, but we have a sample_dim
for the input
. I think the docstring is correct
One more comment for the |
Yes, the Yes, the autoregressive approach of just concatenating conditionals and using one Categorical Net for each dimension would be a really nice feature. |
I think both estimator need an embedding net because these are different embeddings. For the Looking at this now, I think there is a problem: the density estimation build function, e.g., |
Update: The We now build the y-embedding inside of This also enables us to handle |
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.
This is awesome!!!!!
A few comments below, but good to go then!
What does this implement/fix? Explain your changes
A couple of fixes and improvements around MNLE:
CategoricalNet
. There is no need to do tricks with the repetitions in the categorical data (i.e.,log_prob_iid
can be removed (it was not used anyway)).nsf
forMNLE
build_categoricalmassestimator
MNLE
. It did not allow fortheta
embeddings before. Now it does. Importantly, one has to use a "mixed embedding" for the conditioning of the flow because the condition contains embeddedtheta
and "raw" discrete data.Does this close any currently open issues?
Fixes #1134
Fixes #1136
Fixes #1172
Any other comments?
the first commit is for avoiding circular imports.