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

WIP: adding more rules and fixing old ones #80

Merged
merged 15 commits into from Dec 24, 2019

Conversation

shashi
Copy link
Collaborator

@shashi shashi commented Aug 9, 2019

Work in progress for comments!

src/rulesets/Base/base.jl Show resolved Hide resolved
src/rulesets/packages/NaNMath.jl Outdated Show resolved Hide resolved

rrule(::typeof(*), x, y) = x * y, (Rule(ΔΩ -> ΔΩ * y'), Rule(ΔΩ -> x' * ΔΩ))
rrule(::typeof(*), x::Number, y::Number) = x * y, (Rule(ΔΩ -> ΔΩ * y'), Rule(ΔΩ -> x' * ΔΩ))
Copy link
Member

Choose a reason for hiding this comment

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

It's important to think about the complex case here too. All other rrules don't return the complex conjugate of the derivative, so either the conjugate transposes should be changed to unconjugated transposes, or this convention needs to be changed everywhere else

Co-authored-by: YingboMa <mayingbo5@gmail.com>
src/rulesets/Base/base.jl Show resolved Hide resolved
src/rulesets/Base/base.jl Outdated Show resolved Hide resolved
src/rulesets/packages/NaNMath.jl Outdated Show resolved Hide resolved
@nickrobinson251
Copy link
Contributor

Needs tests?

@oxinabox
Copy link
Member

@shashi what is the status with this branch?

@shashi
Copy link
Collaborator Author

shashi commented Aug 29, 2019

I'll not be able to work on this till Sep 1. Will address the comments and add tests.

@willtebbutt
Copy link
Member

@shashi reckon you'll be able to finish this off at some point? Would be really great to have this merged.

@oxinabox
Copy link
Member

I'ld actually really like the tests
Tests for scalar rules are really easy.
Can you add them?

Project.toml Outdated Show resolved Hide resolved
src/rulesets/Base/base.jl Outdated Show resolved Hide resolved
Project.toml Outdated Show resolved Hide resolved
src/rulesets/packages/NaNMath.jl Show resolved Hide resolved
@YingboMa
Copy link
Member

I will add tests in a separate PR.

@oxinabox
Copy link
Member

I will add tests in a separate PR.

Can we add the tests for the functions in src/Base/base.jl in this PR?
I am less fussed about the ones for NaNMath, as we don't have any tests there for that.
But for base.jl we have basically complete coverage.

@YingboMa
Copy link
Member

Sure thing.

@YingboMa
Copy link
Member

Don't merge this yet. Git history is a bit messed up.

@YingboMa YingboMa changed the base branch from master to aa/not-implemented December 23, 2019 20:44
@YingboMa YingboMa changed the base branch from aa/not-implemented to master December 23, 2019 20:44
@YingboMa
Copy link
Member

YingboMa commented Dec 23, 2019

Copy link
Member

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

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

Needs a patch version bump. And a release tagged after merge.

Would be best to add tests for new special functions too.
But if you can't do that now, it's ok to open an issue so it's not forgotten.

@oxinabox
Copy link
Member

Merge when you are ready and do remember to tag a release when you do

@YingboMa YingboMa merged commit 3a16dab into JuliaDiff:master Dec 24, 2019
@YingboMa YingboMa deleted the s/more-rules branch December 24, 2019 18:31
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

7 participants