Add Orthogonal initialization feature. #1496

SomTambe · 2021-02-03T11:09:25Z

As per issue #1431 I have added the Orthogonal matrix initialization feature.

I will add the tests gradually. Just wondering what they can be.

PR Checklist

Tests are added
Entry in NEWS.md
Documentation, if applicable
Final review from @dhairyagandhi96 (for API changes).

johnnychen94

I don't know the details of this initialization algorithm, so I have no idea whether this is coded correctly.

I made some format and style suggestions to it, hope you don't mind. Feel free to ignore it if you think it doesn't make sense.

Edit: tests cases are required.

src/utils.jl

johnnychen94 · 2021-02-03T11:34:51Z

src/utils.jl

+  if rows < cols
+    Q = transpose(Q)
+  end


Another small one-liner trick and feel free to take any of it, or just ignore it.

Suggested change

if rows < cols

Q = transpose(Q)

end

Q = rows < cols ? transpose(Q) : Q

I think I should keep my thing, looks more elegant 😄

I think the reason this strikes several of us as weird is partly that it's not type-stable to re-use Q, not just for different things, but for different types depending on the values of rows, cols. This isn't performance-critical code but that's where everyone's taste was honed.

Again, I would write

return rows > cols ? gain .* M : gain .* transpose(M)

where M is some name for the thing which isn't Q anymore, and the two branches match the branches which generate the random numbers above. They could both be written out on several lines, mat = if rows > cos; randn(... etc, but however they are written, I think they should put the then/else clauses in the same order.

Yeah the mirrored if-else clause are a bit confusing. Should change that if nothing else.

src/utils.jl

mcabbott · 2021-02-03T17:36:09Z

src/utils.jl

+  end
+
+  rows = dims[1]
+  cols = mapreduce(x->x, *, dims; init=1) ÷ rows


This is just div(prod(dims), rows) right?

But why does reshaping this orthogonal matrix make sense? When would that be desirable -- instead of (say) orthogonal per slice, or just an error?

Orthogonal per slice could be done. That is a nice idea indeed.
In the PyTorch implementation, they flatten the matrix if the dimensions are greater than 2. I thought of implementing a similar thing.
What do you think would be better?

I met this idea 5 minutes ago, and haven't read the paper. It seems plausible that orthogonal matrices might be somehow interesting in Dense layers. That only needs ndims=2, and the simplest option would be just to restrict to that. (Would also guarantee that later changes to how you treat 4 dims won't break anything.)

For a Conv layer, my guess would be that orthogonal in the channel dimensions (i.e. the last 2 dims of W). But I've no idea if that's something anyone does. Or possibly they do other things? Could leave until someone needs it.

I wonder why they reshape things in Pytorch, I would have thought the result didn't have any special properties.

I agree with you on the Conv layer point. I believe it should be orthogonal channel-wise. Let me open a new issue on it at the PyTorch repo.

OK. For now, perhaps this should be restricted to matrices? Justorthogonal_rang(rng, rows, cols; gain). This method is probably what you would want to call repeatedly to make higher-dim ones, in any case.

Let's rely on the PyTorch implementation since a large number of people use it. We can always change it later as we feel.

We should not blindly copy them. But if it turns out they have a reason, that may be fine.

src/utils.jl

Added examples rather than plain code. Co-authored-by: Michael Abbott <32575566+mcabbott@users.noreply.github.com>

Co-authored-by: Michael Abbott <32575566+mcabbott@users.noreply.github.com>

src/utils.jl

CarloLucibello · 2021-02-03T19:10:22Z

rename to orthogonal_init or init_orthogonal?

Co-authored-by: Carlo Lucibello <carlo.lucibello@gmail.com>

SomTambe · 2021-02-03T19:15:43Z

@CarloLucibello Since the other initialization techniques do not have any init keyword in their function name, I think it would be better to keep it orthogonal just to be consistent.

src/utils.jl

SomTambe · 2021-02-03T19:20:10Z

Could someone help me find where the docstring is going wrong? I am consistently getting the error Base.Meta.ParseError("unexpected \",\"") here in my docstring, when TravisCI is trying to build my documentation. Thank you!

darsnack · 2021-02-03T20:53:14Z

I also thing the name should be more than orthogonal. The other init functions like "glorot" or "kaiming" are unique to ML parameter initialization but "orthogonal" is used in many contexts. sparse_init is an analogue where a more descriptive name is required.

mcabbott · 2021-02-03T21:07:26Z

Since it behaves a lot like randn, perhaps something like rand_orthogonal or orthogonal_rand?

SomTambe · 2021-02-08T06:30:06Z

@darsnack @DhairyaLGandhi I have made the required changes. All builds passing, only doctest failing which also seems to be from some other set of tests.
I was busy in this weekend, sorry for the delay.

darsnack · 2021-02-08T15:26:15Z

@SomTambe No problem, thanks for putting the effort in to see this through. The only remaining changes are the docstring changes. Do you mind making those?

SomTambe · 2021-02-08T15:55:32Z

@SomTambe No problem, thanks for putting the effort in to see this through. The only remaining changes are the docstring changes. Do you mind making those?

Let me try figuring out what has gone wrong. If you get time, can you mark out in brief what needs to be changed? If you are occupied (which I expect), the general drill will be that I will take some time (I guess till tomorrow morning, since it is night here in India) and I will make the changes 😃 .

SomTambe · 2021-02-08T15:55:37Z

@SomTambe No problem, thanks for putting the effort in to see this through. The only remaining changes are the docstring changes. Do you mind making those?

@darsnack Let me try figuring out what has gone wrong. If you get time, can you mark out in brief what needs to be changed? If you are occupied (which I expect), the general drill will be that I will take some time (I guess till tomorrow morning, since it is night here in India) and I will make the changes 😃 .

darsnack · 2021-02-08T15:57:08Z

They should be marked out in my last review above ☝🏾

darsnack · 2021-02-08T15:59:15Z

Don't worry about the doctest errors btw. The issue is addressed in another PR, we'll either just need to rebase, or we'll merge this PR with the failing test (knowing that it is already fixed in another PR).

darsnack · 2021-02-08T16:00:06Z

~~You'll also need to add orthogonal as a reference in all the other init function's docstrings.~~ See @mcabbott's suggestion here.

darsnack · 2021-02-08T16:34:35Z

Sorry, one more thing! You need to add orthogonal to docs/src/utilities.md.

Add the See Also section. Co-authored-by: Kyle Daruwalla <daruwalla.k.public@icloud.com>

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

src/utils.jl

CarloLucibello · 2021-02-08T20:33:12Z

A very final attempt in favor of the orthogonal_init name, feel free to ignore. Would you ever give this PR the title "Add Orthogonal feature"? I guess not, because it would convey little information about its content. Same for Flux.orthogonal, someone looking at the code wouldn't guess what would be the purpose of this function without looking at the docs

Co-authored-by: Michael Abbott <32575566+mcabbott@users.noreply.github.com>

SomTambe · 2021-02-09T06:59:54Z

@darsnack Made the required changes. I think we should go ahead with orthogonal. All tests passed finally!

darsnack

We still want to switch the docstrings to cross-reference the initialization sections of the docs instead of all-to-all with the other initialization functions.

But this PR is good to go, and we can make that change in a separate PR. Final review from @DhairyaLGandhi needed.

SomTambe · 2021-02-11T13:11:51Z

@DhairyaLGandhi Could you review the request? Maybe merge it?

DhairyaLGandhi

Thanks! This looks great!

bors r+

bors · 2021-02-11T14:03:46Z

Build succeeded:

buildkite/flux-dot-jl

Add Orthogonal initialization feature.

c277919

johnnychen94 reviewed Feb 3, 2021

View reviewed changes

SomTambe added 2 commits February 3, 2021 11:49

Make necessary changes.

8bca693

Make nice changes.

95f94d4

mcabbott reviewed Feb 3, 2021

View reviewed changes

src/utils.jl Outdated Show resolved Hide resolved

mcabbott reviewed Feb 3, 2021

View reviewed changes

src/utils.jl Outdated Show resolved Hide resolved

mcabbott reviewed Feb 3, 2021

View reviewed changes

src/utils.jl Outdated Show resolved Hide resolved

SomTambe and others added 9 commits February 3, 2021 23:27

Update docstring.

338c378

Added examples rather than plain code. Co-authored-by: Michael Abbott <32575566+mcabbott@users.noreply.github.com>

Make citation better.

082a971

Co-authored-by: Michael Abbott <32575566+mcabbott@users.noreply.github.com>

Replace mapreduce thing.

44965f0

Minor docstring change.

4a3d12b

Add tests.

1feb19e

Minor changes.

b2bc5cc

Rectified silly mistakes.

7a84f42

Modified docstring a bit.

28d05df

Minor change.

a8b15d1

CarloLucibello reviewed Feb 3, 2021

View reviewed changes

src/utils.jl Outdated Show resolved Hide resolved

CarloLucibello reviewed Feb 3, 2021

View reviewed changes

src/utils.jl Outdated Show resolved Hide resolved

SomTambe and others added 3 commits February 4, 2021 00:41

Update src/utils.jl

090fd7e

Co-authored-by: Carlo Lucibello <carlo.lucibello@gmail.com>

Removed the unwanted example.

8af7659

Merge branch 'master' of https://github.com/SomTambe/Flux.jl

2735e0c

mcabbott reviewed Feb 3, 2021

View reviewed changes

src/utils.jl Outdated Show resolved Hide resolved

dims::Integer to give better error messages.

64d2e66

Update NEWS.md

a0191a5

SomTambe and others added 5 commits February 8, 2021 17:22

Add orthogonal to docs/src/utilities.md

3e749da

Update src/utils.jl

7a2b610

Add the See Also section. Co-authored-by: Kyle Daruwalla <daruwalla.k.public@icloud.com>

Update src/utils.jl

23a9c5b

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

Update src/utils.jl

691ca35

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

Changed the docs a bit.

786eb8e

mcabbott reviewed Feb 8, 2021

View reviewed changes

src/utils.jl Outdated Show resolved Hide resolved

mcabbott reviewed Feb 8, 2021

View reviewed changes

src/utils.jl Outdated Show resolved Hide resolved

SomTambe and others added 5 commits February 9, 2021 11:44

Update src/utils.jl

54bf710

Co-authored-by: Michael Abbott <32575566+mcabbott@users.noreply.github.com>

Add rng which I had forgotten.

a80bea9

Co-authored-by: Michael Abbott <32575566+mcabbott@users.noreply.github.com>

Slight change

c632cf2

modified tests a bit

2c9f4b8

Rectified mistake.

8f2e4ed

SomTambe requested a review from darsnack February 10, 2021 08:58

darsnack approved these changes Feb 10, 2021

View reviewed changes

DhairyaLGandhi approved these changes Feb 11, 2021

View reviewed changes

CarloLucibello mentioned this pull request Feb 11, 2021

PyTorch feature parity #1431

Open

92 tasks

bors bot merged commit 4c53672 into FluxML:master Feb 11, 2021

darsnack mentioned this pull request Jan 27, 2022

Add orthogonal kernel initialization for recurrent cells #1171

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Orthogonal initialization feature. #1496

Add Orthogonal initialization feature. #1496

SomTambe commented Feb 3, 2021 •

edited

Loading

johnnychen94 left a comment •

edited

Loading

johnnychen94 Feb 3, 2021

SomTambe Feb 3, 2021

mcabbott Feb 8, 2021 •

edited

Loading

darsnack Feb 8, 2021

mcabbott Feb 3, 2021

SomTambe Feb 3, 2021

mcabbott Feb 3, 2021 •

edited

Loading

SomTambe Feb 4, 2021

mcabbott Feb 4, 2021

SomTambe Feb 4, 2021

mcabbott Feb 4, 2021

CarloLucibello commented Feb 3, 2021

SomTambe commented Feb 3, 2021

SomTambe commented Feb 3, 2021 •

edited

Loading

darsnack commented Feb 3, 2021 •

edited

Loading

mcabbott commented Feb 3, 2021

SomTambe commented Feb 8, 2021 •

edited

Loading

darsnack commented Feb 8, 2021

SomTambe commented Feb 8, 2021

SomTambe commented Feb 8, 2021 •

edited

Loading

darsnack commented Feb 8, 2021

darsnack commented Feb 8, 2021

darsnack commented Feb 8, 2021 •

edited

Loading

darsnack commented Feb 8, 2021

CarloLucibello commented Feb 8, 2021

SomTambe commented Feb 9, 2021

darsnack left a comment •

edited

Loading

SomTambe commented Feb 11, 2021

DhairyaLGandhi left a comment

bors bot commented Feb 11, 2021

Add Orthogonal initialization feature. #1496

Add Orthogonal initialization feature. #1496

Conversation

SomTambe commented Feb 3, 2021 • edited Loading

PR Checklist

johnnychen94 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcabbott Feb 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcabbott Feb 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarloLucibello commented Feb 3, 2021

SomTambe commented Feb 3, 2021

SomTambe commented Feb 3, 2021 • edited Loading

darsnack commented Feb 3, 2021 • edited Loading

mcabbott commented Feb 3, 2021

SomTambe commented Feb 8, 2021 • edited Loading

darsnack commented Feb 8, 2021

SomTambe commented Feb 8, 2021

SomTambe commented Feb 8, 2021 • edited Loading

darsnack commented Feb 8, 2021

darsnack commented Feb 8, 2021

darsnack commented Feb 8, 2021 • edited Loading

darsnack commented Feb 8, 2021

CarloLucibello commented Feb 8, 2021

SomTambe commented Feb 9, 2021

darsnack left a comment • edited Loading

Choose a reason for hiding this comment

SomTambe commented Feb 11, 2021

DhairyaLGandhi left a comment

Choose a reason for hiding this comment

bors bot commented Feb 11, 2021

SomTambe commented Feb 3, 2021 •

edited

Loading

johnnychen94 left a comment •

edited

Loading

mcabbott Feb 8, 2021 •

edited

Loading

mcabbott Feb 3, 2021 •

edited

Loading

SomTambe commented Feb 3, 2021 •

edited

Loading

darsnack commented Feb 3, 2021 •

edited

Loading

SomTambe commented Feb 8, 2021 •

edited

Loading

SomTambe commented Feb 8, 2021 •

edited

Loading

darsnack commented Feb 8, 2021 •

edited

Loading

darsnack left a comment •

edited

Loading