Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
LeakyReLU #81
LeakyReLU #81
Changes from 7 commits
1e27430
180a527
0f5200a
7d36fde
533b86e
fd2e33b
8b69cab
ba299ff
9b4d0d6
dbeb414
7771155
6e9d9f7
18e30f9
c5b058b
c72fd11
68d3518
c14a5db
4a21b73
b4119fd
1b97a0f
1ebd9ba
07cc632
f2f167e
003dfb6
e17d5d7
a83cb7b
28158ae
6077578
410166b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Maybe
would be more efficient (since it avoids the allocation of
mask
) and more stable (since it does not require computations based on types).As a side remark, IMO it is a bit unfortunate that currently so many almost identical implementations of a function are needed to define a bijector. Maybe this could be resolved by defining defaults on a high level for basically all bijectors (or a subtype of BaseBijectors, similar to BaseKernels in KernelFunctions) that call just in one or two methods that users would have to implement if their bijectors belong to these simple standard groups. With Julia >= 1.3 it is also possible to define methods for abstract functions such as
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.
Won't this call
zero(x)
n times though, i.e. have the same allocation? Other than that, it seems like a good idea 👍100%.
I'm unaware of what KernelFunctions.jl does. Are they simply not making the
struct
callable? If so, it was a deliberate design-decision to go with making structs callable when we started out. We were debating whether or not to do this or include sometransform
method so we could define "one method to rule them all" on an abstract type. Ended up going with the "callable struct" approach, but it def has it's issues, i.e. redundant code.I've recently played around a bit with actually using a
transform
method under the hood, but adding a macro which allows you to say@deftransform function transform(b, x) ... end
and we just add a(b::Bijector)(x) = transform(b, x)
just after the method declaration. This would also allow us to implement in-place versions of all the methods, i.e.transform!(b, x, out)
,logabsdetjac(b, x, out)
, and so on. Thoughts?Woah, really? If so that would solve the problem, no?
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.
No, KernelFunctions API is based on callable structs only (using a function was removed a while ago). But e.g. translation-invariant kernels are built all in the same way, usually they use some simple function to evaluate the distance between the inputs (e.g. using Distances) and then they apply some nonlinear mapping afterwards. With this special structure also all kind of optimizations are possible when constructing kernel matrices etc. Hence there is a special type of such kernels (SimpleKernel IIRC), and then users just define their metric and the nonlinear mapping and get everything else for free. There's more information here: https://juliagaussianprocesses.github.io/KernelFunctions.jl/dev/create_kernel/
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.
To add on to the above, because it sucks to have to implement
(b::Bijector)
,logabsdetjac
and then finallyforward
, would it be an idea to add a macro that would allow you to define all in one go?EDIT: Just see #137 :)
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.
Ah, I see. I don't think it's so easy to do for bijectors as we have much more structure than just "forward" evaluation, no?
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 think my only real comment here is on testing other AD backends like ReverseDiff, but I'm not sure how important that is 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.
There are some tests for it in
test/bijectors/interface.jl
. But yeah, testing in Bijectors is honestly a bit of mess atm. In a couple of the other PRs I've added some functionality which makes it easier to use a "standardized" testing suite for a newBijector
, so the plan is to use that in the future 👍