-
Notifications
You must be signed in to change notification settings - Fork 34
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
Remove ADBackend and ADBijector #242
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.
I have never used it, so I'm fine with removing it 😛
I wonder though if it is a reasonably common thing users might want to use, if we should add an example without ADBijector
to the README and/or to the tests.
@torfjelde can you resolve the merge conflicts introduced by #214? |
Aight, tests should now be passing. BUT shouldn't this also technically be another breaking change? I.e. we should bump to 0.12? It's a bit annoying given that we just made a breaking release, but seems like the right course of action IMO. @devmotion @yebai |
Yes, it removes exported and documented functionality, so IMO it deserves another breaking release. I don't think it's too bad as the number of direct dependents is fairly low: https://juliahub.com/ui/Packages/Bijectors/39uFz/0.10.6?page=2 |
Version bumped. Should be ready to go once tests pass. |
thanks @torfjelde and @devmotion! |
ADBijector
is, AFAIK, never used. In addition, it's a very trivial thing to implement on your own.Because of this, I feel like it's unnecessary to add all the additional complexity that is
ADBackend
just for the sake of maybe one or two use-cases ofADBijector
. Hence I think we should just drop it as part of the breaking release.