Skip to content
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

Making dimensionality part of the Bijector type #44

Merged
merged 82 commits into from
Nov 7, 2019

Conversation

torfjelde
Copy link
Member

@torfjelde torfjelde commented Sep 13, 2019

Overview

This PR attempts to solve #35 by introducing the dimensionality of input/output (the same) N into the Bijector type.

Pros:

  1. Solves ambiguities referred to in Bijector support for batch computation #35
  2. Catches dimension-mismatch in compositions at compile-time
  3. Most standard subtypes of Bijector{N} are only well-defined for a specific value of N, so in these cases the only change necessary is, say, Specific1DBijector <: BijectorSpecific1DBijector <: Bijector{1}. E.g. SimplexBijector is only well-defined for vector input, so SimplexBijector <: Bijector{1}. To get a quick overview of the differences from a user-perspective, have a look at the diff for tests/interface.jl. There's barely any change.
  4. Batch computation now works nicely:)

Cons:

  1. Log() looks nicer than Log{0}(), etc. But again, due to (3) in pros this is in most cases not a huge issue from a user perspective.
  2. Mismatches in sizes are not caught at the moment. Though we can of course add run-time checks for this.

To-be discussed

  • Is this really the best solution to make batch-computation work? It does put a bit more work on the user for custom Bijector since it requires explicit implementation of logabsdetjac for both standard case and the batch computation. But of course if the user doesn't want batch-computation, then there's no more work than before.
  • Is this design more restrictive in what we can do with Bijector? Could people want to implement something more general than just standard Real or AbstractArray, and this prohibits it? If so, is this of any priority to us?
  • Something similar to StaticArrays.jl was suggested to catch size-mismatches at compile time. After thinking about this for a bit I came to the conclusion that it would get real nasty real quick and would be much more restrictive than we'd like. Also not entirely sure how to go about implementing this as I haven't looked properly into the src of StaticArrays.jl yet.

TODOs

  • Update README.md
  • Update docstrings
  • Conclude discussions:)

src/interface.jl Outdated Show resolved Hide resolved
src/interface.jl Show resolved Hide resolved
src/interface.jl Outdated Show resolved Hide resolved
src/interface.jl Outdated Show resolved Hide resolved
src/interface.jl Outdated Show resolved Hide resolved
src/interface.jl Show resolved Hide resolved
src/interface.jl Show resolved Hide resolved
@torfjelde
Copy link
Member Author

Btw, I think it would be a good idea to separate implementations of Bijector into their separate files, e.g. instead of implementing Logit and SimplexBijector in the same file src/interface.jl, we move them to src/bijectors/logit.jl and src/bijectors/simplex.jl, respectively.

I have several new bijectors in the pipeline and IMO such a project structure makes much more sense as the number of bijectors grows.

Question: should I include this in this PR or do it in a separate PR?

@xukai92
Copy link
Member

xukai92 commented Nov 5, 2019

I have several new bijectors in the pipeline and IMO such a project structure makes much more sense as the number of bijectors grows.

Definitely

Question: should I include this in this PR or do it in a separate PR?

I'm fine if you do it in this PR. But you might need to resolve some conflicts to make other PRs merge afterwards.

@torfjelde
Copy link
Member Author

I'm fine if you do it in this PR. But you might need to resolve some conflicts to make other PRs merge afterwards.

Then I'll do that. And yeah, but probably going to have to do that anyways!:)

@torfjelde
Copy link
Member Author

torfjelde commented Nov 5, 2019

Then I'll do that. And yeah, but probably going to have to do that anyways!:)

Actually, let's just get this thing merged and then we can do that after. There's so much work that depends on this PR that I'd like to just get it merged asap

@xukai92
Copy link
Member

xukai92 commented Nov 5, 2019

There some conflicts need to be resolved. After that is solved I'm happy to merge it given CI passes.

@xukai92
Copy link
Member

xukai92 commented Nov 6, 2019

The only faild CIs are in the nightly Julia version. I guess it's fine to merge it now. I will do so before I sleep if no objection.

@yebai yebai changed the title [WIP] Making dimensionality part of the Bijector type Making dimensionality part of the Bijector type Nov 6, 2019
@torfjelde
Copy link
Member Author

The only faild CIs are in the nightly Julia version. I guess it's fine to merge it now. I will do so before I sleep if no objection.

Yeah, from a quick look at the logs it seems like it's an issue with Tracker.jl rather than us so will probably be resolved in the future by updating deps.

@xukai92 xukai92 merged commit 366cc06 into TuringLang:master Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants