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

Add rrule for Diagonal * AbstractVector #108

Merged
merged 3 commits into from Sep 21, 2019
Merged

Conversation

tkf
Copy link
Contributor

@tkf tkf commented Sep 20, 2019

ref #52 (comment)

Reading #103 and other source code, I suppose we are only defining rrules for Real element types at the moment so didn't put adjoint in the definition. (By the way, if that's the case, I think it makes sense to mention it in #103.)

@oxinabox
Copy link
Member

I suppose we are only defining rrules for Real element types at the moment so didn't put adjoint in the definition. (By the way, if that's the case, I think it makes sense to mention it in #103.)

it is https://github.com/JuliaDiff/ChainRules.jl/pull/103/files#diff-493d89206c11fea99259d0771034d26aR283

We explictly don't conjugate inside rules.

rng, N = MersenneTwister(123456), 3
rrule_test(*, randn(rng, N),
(Diagonal(randn(rng, N)), Diagonal(randn(rng, N))),
(randn(rng, N), randn(rng, N)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistancy with the rest of the code, the intentation style is:

        rrule_test(
            *, randn(rng, N),
            (Diagonal(randn(rng, N)), Diagonal(randn(rng, N))),
            (randn(rng, N), randn(rng, N)),
        )

i.e. always start and end a multiline call with a line that has no arguments on it,
and always ident by 4, rather than indenting to line-up with the brace

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually it would be with one argument per line

rrule_test(
    *,
    randn(rng, N),
    (Diagonal(randn(rng, N)), Diagonal(randn(rng, N))),
    (randn(rng, N), randn(rng, N)),
)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but I feel like this is one of the cases where the clear relationship between
*, randn(rng, N), as inputs to the function makes it worth putting them on one-line.

I'ld be happy with either

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know that ChainRules.jl is using a sane code style!

@tkf
Copy link
Contributor Author

tkf commented Sep 20, 2019

OK. I think I reflected the comments in the new version. Travis is happy with it.

@oxinabox oxinabox merged commit 4afdfdd into JuliaDiff:master Sep 21, 2019
@oxinabox
Copy link
Member

Thanks

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.

None yet

4 participants