Fix #1556 #1557

DhairyaLGandhi · 2021-03-31T16:22:52Z

Reverts some Dense changes since those caused issues downstream. cc @ChrisRackauckas @darsnack @CarloLucibello

DhairyaLGandhi · 2021-03-31T16:24:01Z

It needs some fixes to the tests to make sure we are getting expected results. This is a technically breaking change now.

darsnack

I don't think this is the correct way to fix #1556. It is not necessary to revert the constructor work. Beyond necessitating a breaking change, most of the stuff being reverted here is actually good work that should be kept. Let's not do drastic changes to the codebase for bug fixes if it is not necessary. If there's a desire to do a design refactor, then that should be done in a separate PR. Happy for us to have a design discussion, but in this case, we can release the appropriate fix without a design overhaul.

A proper fix for #1556 will involve just deleting the eltype check from create_bias.

darsnack · 2021-03-31T16:40:22Z

To be clear, it isn't create_bias or the Dense changes that caused this issue. create_bias was added in #1379, and the erroneous eltype check was added in #1440. All the changes in #1440 can be kept without the unnecessary check, and create_bias is reverted to its prior behavior.

DhairyaLGandhi · 2021-03-31T16:50:33Z

I'll do a more minimal change. What specific changes would you like retained?

I would also prefer for some changes to our design to simplify its implementation. Removing create_bias from here doesn't seem too disruptive to me. Good call on fixing the bad check too.

darsnack · 2021-03-31T16:58:39Z

My preference would be to keep everything except delete this check code and just return bias. We could remove create_bias, but to do it in a non-breaking way that keeps the current API means adding a bias isa Bool check to all the constructors with a bias. create_bias should really be an internal utility cause that's how it is used.

One other possible change is the specification of W in Dense to be <:AbstractMatrix. My gut says this constraint should be lifted and allowed to be <:AbstractArray. But I haven't hunted down where and why that change happened, so maybe it is necessary. Possibly @CarloLucibello or @mcabbott can shed some light.

I do agree with you that we can/should think about a better design that doesn't require an internal utility to handle the bias argument. But my feeling is that it would involve a discussion on Flux.Zeros, so that's why I am suggesting opening a separate issue/PR for that refactor.

darsnack · 2021-03-31T17:00:54Z

It's possible there is more than the change I suggested to be reverted, but can we approach the PR from the opposite direction? i.e. restricting the changes to what I suggest, then slowly reverting more stuff as necessary?

mcabbott · 2021-03-31T17:36:57Z

No time today but agree that if trying to fix something quickly, the minimal change is best:

My preference would be to keep everything except delete this check code and just return bias.

That seems to be what's actually causing problems, right?

One other possible change is the specification of W in Dense to be <:AbstractMatrix. My gut says this constraint should be lifted and allowed to be <:AbstractArray.

Dense calls W * x, are there other ndims(W) this can sensibly work with?

DhairyaLGandhi · 2021-03-31T17:48:48Z

The thinking is thus:

remove the check in create_bias
remove the conversion

Removing the function from here basically means we don't have to worry about its behaviour in a corner case, and the package becomes more reliable for downstream users. We aren't losing features here either.

DhairyaLGandhi · 2021-03-31T18:00:24Z

@darsnack the layer is back to where it was, the tests are back to their earlier state as well.

darsnack

Looking close to me. Can you test the snippet @ChrisRackauckas provided in #1556 to see if it is resolved?

Dense calls W * x, are there other ndims(W) this can sensibly work with?

Probably not, but I guess our preference is to not pre-constrain. I think the change to <:AbstractArray makes sense. W * x will throw an error on the forward pass, or the output passed into the next layer will throw a dimension mismatch. As long as we don't have a silent error, then I think it might better to leave it open.

src/layers/basic.jl

Co-authored-by: Kyle Daruwalla <daruwalla.k.public@icloud.com>

DhairyaLGandhi · 2021-03-31T18:30:23Z

The MWE works

darsnack

Looking correct to me.

test/utils.jl

DhairyaLGandhi · 2021-03-31T21:29:12Z

bors r+

@ChrisRackauckas

1557: Fix #1556 r=DhairyaLGandhi a=DhairyaLGandhi Reverts some Dense changes since those caused issues downstream. cc @ChrisRackauckas @darsnack @CarloLucibello Co-authored-by: Dhairya Gandhi <dhairya@juliacomputing.com>

DhairyaLGandhi · 2021-03-31T21:34:10Z

bors ping

bors · 2021-03-31T21:34:11Z

pong

mcabbott · 2021-03-31T21:52:43Z

IMO this should be reverted. Changes orthogonal to fixing a pressing issue don't need to be rushed in.

bors · 2021-03-31T21:57:44Z

This PR was included in a batch that successfully built, but then failed to merge into master (it was a non-fast-forward update). It will be automatically retried.

CarloLucibello · 2021-03-31T23:51:48Z

test/layers/basic.jl

@@ -40,16 +40,17 @@ import Flux: activations
      @test Dense(rand(Float16, 100,10), true).bias isa Vector{Float16}  # creates matching type
      @test_skip Dense(rand(Float16, 100,10), rand(100)).bias isa Vector{Float16}  # converts to match

-      @test Dense(3,4; init=Base.randn, bias=true).bias isa Vector{Float64}
+      @test_skip Dense(3,4; init=Base.randn, bias=true).bias isa Vector{Float64}


Is this now producing a float32 bias? this is no good.

Since there is no indication of the type of bias, it would make sense to produce single precision bias. It is also addressed in #1559 to restore backwards compatibility with the behaviour.

1561: Reverts #1557 r=DhairyaLGandhi a=DhairyaLGandhi Reverts #1557 Co-authored-by: Dhairya Gandhi <dhairya@juliacomputing.com>

DhairyaLGandhi added 2 commits March 31, 2021 20:22

fix #1556

90aab22

check for bias flag

21ca0e4

darsnack requested changes Mar 31, 2021

View reviewed changes

DhairyaLGandhi added 2 commits March 31, 2021 22:21

more minimal change

b5beeac

typo

43a025e

don't convert bias type

b92e598

minimal changes

ab508e0

reinstate some deprecations

bc9e6d8

move method back

1abd371

darsnack requested changes Mar 31, 2021

View reviewed changes

src/layers/basic.jl Outdated Show resolved Hide resolved

src/layers/basic.jl Outdated Show resolved Hide resolved

src/layers/basic.jl Outdated Show resolved Hide resolved

DhairyaLGandhi and others added 4 commits March 31, 2021 23:43

use consistent ones

714a37f

qualify depwarns

9f493fd

Apply suggestions from code review

1123177

Co-authored-by: Kyle Daruwalla <daruwalla.k.public@icloud.com>

type annotations

221849f

darsnack reviewed Mar 31, 2021

View reviewed changes

test/utils.jl Outdated Show resolved Hide resolved

rm change

abd324e

mcabbott mentioned this pull request Mar 31, 2021

Minimal fix of #1556, remove eltype checks #1558

Merged

DhairyaLGandhi added 4 commits April 1, 2021 00:39

comment out some tests

bd889ca

typo

66a1d8b

comment out couple more outliers

e9bd21f

missing test

0a35664

DhairyaLGandhi closed this in #1558 Mar 31, 2021

DhairyaLGandhi reopened this Mar 31, 2021

Merge branch 'master' into dg/den

35d737b

DhairyaLGandhi merged commit f352374 into master Mar 31, 2021

darsnack added a commit to darsnack/Flux.jl that referenced this pull request Mar 31, 2021

Revert FluxML#1557 temporarily

94d9d56

darsnack mentioned this pull request Mar 31, 2021

Revert #1557 temporarily #1560

Closed

4 tasks

CarloLucibello reviewed Mar 31, 2021

View reviewed changes

DhairyaLGandhi mentioned this pull request Apr 1, 2021

Reverts #1557 #1561

Merged

bors bot added a commit that referenced this pull request Apr 1, 2021

Merge #1561

59f9b12

1561: Reverts #1557 r=DhairyaLGandhi a=DhairyaLGandhi Reverts #1557 Co-authored-by: Dhairya Gandhi <dhairya@juliacomputing.com>

bors bot added a commit that referenced this pull request Apr 1, 2021

Merge #1561

8a5b977

1561: Reverts #1557 r=DhairyaLGandhi a=DhairyaLGandhi Reverts #1557 Co-authored-by: Dhairya Gandhi <dhairya@juliacomputing.com>

CarloLucibello deleted the dg/den branch April 7, 2022 07:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1556 #1557

Fix #1556 #1557

DhairyaLGandhi commented Mar 31, 2021 •

edited

Loading

DhairyaLGandhi commented Mar 31, 2021

darsnack left a comment •

edited

Loading

darsnack commented Mar 31, 2021

DhairyaLGandhi commented Mar 31, 2021

darsnack commented Mar 31, 2021

darsnack commented Mar 31, 2021

mcabbott commented Mar 31, 2021

DhairyaLGandhi commented Mar 31, 2021

DhairyaLGandhi commented Mar 31, 2021

darsnack left a comment

DhairyaLGandhi commented Mar 31, 2021

darsnack left a comment

DhairyaLGandhi commented Mar 31, 2021

DhairyaLGandhi commented Mar 31, 2021

bors bot commented Mar 31, 2021

mcabbott commented Mar 31, 2021

bors bot commented Mar 31, 2021

CarloLucibello Mar 31, 2021

DhairyaLGandhi Apr 6, 2021

Fix #1556 #1557

Fix #1556 #1557

Conversation

DhairyaLGandhi commented Mar 31, 2021 • edited Loading

DhairyaLGandhi commented Mar 31, 2021

darsnack left a comment • edited Loading

Choose a reason for hiding this comment

darsnack commented Mar 31, 2021

DhairyaLGandhi commented Mar 31, 2021

darsnack commented Mar 31, 2021

darsnack commented Mar 31, 2021

mcabbott commented Mar 31, 2021

DhairyaLGandhi commented Mar 31, 2021

DhairyaLGandhi commented Mar 31, 2021

darsnack left a comment

Choose a reason for hiding this comment

DhairyaLGandhi commented Mar 31, 2021

darsnack left a comment

Choose a reason for hiding this comment

DhairyaLGandhi commented Mar 31, 2021

DhairyaLGandhi commented Mar 31, 2021

bors bot commented Mar 31, 2021

mcabbott commented Mar 31, 2021

bors bot commented Mar 31, 2021

CarloLucibello Mar 31, 2021

Choose a reason for hiding this comment

DhairyaLGandhi Apr 6, 2021

Choose a reason for hiding this comment

DhairyaLGandhi commented Mar 31, 2021 •

edited

Loading

darsnack left a comment •

edited

Loading