-
Notifications
You must be signed in to change notification settings - Fork 750
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
NanMixture: Distribution to model missing values #913
NanMixture: Distribution to model missing values #913
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.
Thanks Pascal, looks great overall. I see there are some TODOs here and there... are these things you are working on including now?
lambda value: value, value=self.value, num_samples=num_samples | ||
) | ||
|
||
def quantile(self, level: Tensor) -> Tensor: |
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.
In a deterministic distribution, shouldn't all quantiles be the same, i.e., the value of the distribution?
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.
Mostly, but there are some edge cases: The quantile of p=0 is for example -inf. Furthermore, if the deterministic distribution is NaN-valued the quantile is always NaN.
Thank you for reviewing, Konstantinos. |
|
||
loss_value = loss.mean().asnumpy() | ||
t.set_postfix({"loss": loss_value}) | ||
trainer.step(BATCH_SIZE) |
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 just an idea (and we could do this in a separate PR): we know that EM works better than SGD for the job, would it make sense to use EM in this type of tests? As long as this would involve invoking the loss (and its backward) then it should be fine. In that case we could probably hope for “stricter” assertions on the recovered parameters 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.
We could do that for the Mixture distribution. However, for the NanMixture the maximisation step is not defined. Also, EM does not use the gradient. Maybe in a separate PR I could implement something like "A Gradient Algorithm Locally Equivalent to the EM Algorithm" which uses the gradient and would, I think, work for the NanMixture.
We can also achieve stricter assertions by using a larger sample size, which however makes the test slower.
* added a deterministic/degenerate distribution * Corrected the formula of the Standard Deviation of the Mixture Distribution * Added a distribution to model missing values * added test script to test log_prob and the gradients and fixed some edge cases * fixed bug in test script * corrected true gradients in the test file and edge cases of the log_prob calculations * fixed edge cases of the gradients * bugfix CategoricalOutput * test skip * style fix * addressed PR issues * skipping tests which take too long and rearranging imports * fixed the output args issue of a NanMixture with a Categorical distribution * lowered assertion tolerances * lowered assertion tolerances * refractoring of the NanMixture tests * refractoring of the NanMixture tests * added NanMixture support to the SimpleFeedForward and fixed typing issue * refractoring * removing SimpleFeedForward changes * increased sample size for mixture stddev and mean tests to prevent false alarms * added random seeds to tests * increased tol Co-authored-by: Lorenzo Stella <lorenzostella@gmail.com>
Description of changes:
Added a distribution to model data which contains missing values.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.