-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Deprecate remaining VI methods #5581
Comments
CC @ferrine, @fonnesbeck, @twiecki |
My vote is on removing them, don't think anyone uses those. If they do, they'll probably complain. We could ask on discourse too. |
I'm pretty sure OVPI can go, but normalizing flows are something we would want. They are a pretty useful and are available in most VI implementations these days (e.g. pyro, TF). That said, they can always be added back if anyone ever wants to implement them. |
I'd vote for removal, agree with @fonnesbeck. This functionality did not prove to be super useful. |
@ferrine do you think it will be straightforward to get normalizing flows ported? Were they fully functional in v3? |
they were, but that time I struggled with shape inference for parameter creation. In v3 I relied on test values for NF, now test values are less strict and it broke the internals. To fix this issue new mechanism has to be implemented to initialize normalizing flows. |
To summarize there are 3 blocks of features
|
So shall we delete everything and open an issue to re-implement normalizing flows in the future (either here or in pymc-experimental)? |
That sounds good to me. |
Yes, seems sensible, assuming "everything" is all methods with NotImplementedInference exceptions. |
I would like to work on this. |
@purna135 Great, just open a draft PR when you have a start so that others can see that you're working on this. |
Sure @twiecki, Thank You |
#4582 left some old VI methods marked as NotImplemented
Normalizing flows:
pymc/pymc/variational/approximations.py
Line 363 in ef5f91b
And these:
pymc/pymc/variational/opvi.py
Lines 959 to 962 in e987950
We should decide whether we want to refactor or remove them
The text was updated successfully, but these errors were encountered: