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

Restrict the scalar multiplication rules to Numbers #77

Merged
merged 1 commit into from Aug 10, 2019

Conversation

shashi
Copy link
Collaborator

@shashi shashi commented Aug 9, 2019

Before this, e.g. frule(*, [1,2], Dual(1)) would return a non-nothing. This change helps avoid special casing this in a generic implementation of scalar diff.

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.

Yeah this was concerning.

@oxinabox
Copy link
Member

oxinabox commented Aug 9, 2019

Will merge once CI passes.

@nickrobinson251
Copy link
Contributor

Shall we add a test, eg that the example in the OP throws a MethodError, just so this doesn't get reverted?

@oxinabox
Copy link
Member

oxinabox commented Aug 9, 2019

Shall we add a test, eg that the example in the OP throws a MethodError, just so this doesn't get reverted?

It doesn't seem needful, and it is hard to qualify exactly what that case is, except via very much special casing.
E,g, there are (2?) other Dual numbers that are subtypes of Number in julia.
So that test would boil down to

struct MyNonNumber
x
end

@testset "Don't just do scalar multiplication without any type constraints (#77)" begin
    @test nothing === frule(*, [1,2], MyNonNumber(1))
end

@shashi do you think we should add that?
I guess now that I have written it...

@willtebbutt
Copy link
Member

Would be good to have the various permutations of @oxinabox 's suggestion for non-number arguments, as well as for the rrule

@oxinabox
Copy link
Member

oxinabox commented Aug 9, 2019

could add it to test/test_utils.jl

@oxinabox
Copy link
Member

Ok I have openned an issue, re:tests
So can merge this

@oxinabox oxinabox merged commit 4303bb8 into JuliaDiff:master Aug 10, 2019
@shashi shashi deleted the s/mul-scalar branch August 16, 2019 01:42
@nickrobinson251 nickrobinson251 added the type constraints Potentially raises a question about how tightly to constrain argument types for a rule. See #232 label Jan 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type constraints Potentially raises a question about how tightly to constrain argument types for a rule. See #232
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants