Factor out bnv derivs #137

rgiordan · 2016-03-07T22:03:16Z

This is a first step towards fitting the PSF in Celeste. I'd like to submit the PR now to avoid nasty merges while I finish the rest. Tests pass on my machine, but currently waiting on Travis.

jeff-regier · 2016-03-07T23:35:34Z

Have/will you re-run the benchmark script to make sure the changes haven't slowed down the objective function evaluations?

Factor out bnv derivs

kbarbary · 2016-03-08T18:39:22Z

src/bivariate_normals.jl

        the_mean::Vector{T1}, the_cov::Matrix{T2}, weight::T3;
-        calculate_siginv_deriv::Bool=true)
+        calculate_siginv_deriv::Bool=true) = begin


Let's use function syntax everywhere for functions rather than begin/end blocks.

Agreed. Sorry about this, I just missed this in the merge.

On Tue, Mar 8, 2016 at 10:39 AM, Kyle Barbary notifications@github.com
wrote:

In src/bivariate_normals.jl
#137 (comment):

the_mean::Vector{T1}, the_cov::Matrix{T2}, weight::T3;

calculate_siginv_deriv::Bool=true)

calculate_siginv_deriv::Bool=true) = begin

Let's use function syntax everywhere for functions rather than begin/end
blocks.

—
Reply to this email directly or view it on GitHub
https://github.com/jeff-regier/Celeste.jl/pull/137/files#r55405316.

kbarbary · 2016-03-08T18:49:45Z

The file SensitiveFloat.jl is a duplicate of SensitiveFloats.jl. The rename to plural was done in #134 -- necessary because SensitiveFloats is now a module and has to be plural to avoid name collision with the type SensitiveFloat. (This sort of pluralization is typical in Julia module names for this reason.)

kbarbary · 2016-03-08T18:51:46Z

src/SensitiveFloat.jl

+""" ->
+type SensitiveFloat{ParamType <: CelesteTypes.ParamSet, NumType <: Number}
+    # Actually a single value, but an Array to avoid memory allocation
+    v::Vector{NumType}


While we're looking at SensitiveFloat, I wanted to ask why v is an array instead of just v::Numtype here. The effect of using an array is opposite of what the comment implies.

I found when looking at memory allocation for the full Hessian that memory
was allocated each time the value of a SensitiveFloat was changed and (in
some cases at least) even accessed. I don't know enough about the julia
compiler to account for this, but when I switched to using an array for the
value the memory allocation disappeared. So when I say "to avoid memory
allocation" I mean "to avoid memory allocation whenever the value is used
or changed" rather than to avoid memory allocation up front.

On Tue, Mar 8, 2016 at 10:51 AM, Kyle Barbary notifications@github.com
wrote:

In src/SensitiveFloat.jl
#137 (comment):

+A function value and its derivative with respect to its arguments.
+
+Attributes:

v: The value

d: The derivative with respect to each variable in

P-dimensional VariationalParams for each of S celestial objects

in a local_P x local_S matrix.

h: The second derivative with respect to each variational parameter,

in the same format as d. This is used for the full Hessian

with respect to all the sources.

hs: An array of per-source Hessians. This will generally be reserved

for the Hessian of brightness values that depend only on one source.
+""" ->
+type SensitiveFloat{ParamType <: CelesteTypes.ParamSet, NumType <: Number}

Actually a single value, but an Array to avoid memory allocation

v::Vector{NumType}

While we're looking at SensitiveFloat, I wanted to ask why v is an array
instead of just v::Numtype here. The effect of using an array is opposite
of what the comment implies.

—
Reply to this email directly or view it on GitHub
https://github.com/jeff-regier/Celeste.jl/pull/137/files#r55407321.

Any luck duplicating the phenomenon---memory allocation on access---in a minimal working example?

I didn't try. Do you think it's important?

On Tue, Mar 8, 2016 at 12:04 PM, Jeffrey Regier notifications@github.com
wrote:

In src/SensitiveFloat.jl
#137 (comment):

+A function value and its derivative with respect to its arguments.
+
+Attributes:

v: The value

d: The derivative with respect to each variable in

P-dimensional VariationalParams for each of S celestial objects

in a local_P x local_S matrix.

h: The second derivative with respect to each variational parameter,

in the same format as d. This is used for the full Hessian

with respect to all the sources.

hs: An array of per-source Hessians. This will generally be reserved

for the Hessian of brightness values that depend only on one source.
+""" ->
+type SensitiveFloat{ParamType <: CelesteTypes.ParamSet, NumType <: Number}

Actually a single value, but an Array to avoid memory allocation

v::Vector{NumType}

Any luck duplicating the phenomenon---memory allocation on access---in a
minimal working example?

—
Reply to this email directly or view it on GitHub
https://github.com/jeff-regier/Celeste.jl/pull/137/files#r55418802.

It might turn out the be nothing, but it's kind of unsettling. Could it be related to #125 ?

Typically, if there's no type instability, modifying a bitstype field like Float64 should not allocate. Example:

julia> type A val::Float64 end julia> function update!(a) for i=1:10_000_000 a.val = rand() end end julia> a = A(0.0); julia> @time update!(a) 0.048819 seconds (19.14 k allocations: 826.487 KB) julia> @time update!(a) 0.039082 seconds (4 allocations: 160 bytes)

I don't know whether this is absolutely always true though.

The problem occurred in accumulate_source_brightness!. I also recall that
before I refactored ElboDeriv, there was a construction where we defined

fsm = (fsm[1], fsm[2])

and then referred to

fsm[i].v

Is it possible that the construction of this tuple was causing some type
instability that led to this memory allocation? I do think that not every
reference to a SF's value caused allocation. This is interesting if
someone wants to try switching back and looking at memory allcoation, but I
do think there are bigger fish to fry before the SC deadline.

On Tue, Mar 8, 2016 at 12:46 PM, Kyle Barbary notifications@github.com
wrote:

In src/SensitiveFloat.jl
#137 (comment):

+A function value and its derivative with respect to its arguments.
+
+Attributes:

v: The value

d: The derivative with respect to each variable in

P-dimensional VariationalParams for each of S celestial objects

in a local_P x local_S matrix.

h: The second derivative with respect to each variational parameter,

in the same format as d. This is used for the full Hessian

with respect to all the sources.

hs: An array of per-source Hessians. This will generally be reserved

for the Hessian of brightness values that depend only on one source.
+""" ->
+type SensitiveFloat{ParamType <: CelesteTypes.ParamSet, NumType <: Number}

Actually a single value, but an Array to avoid memory allocation

v::Vector{NumType}

Typically, if there's no type instability, modifying a bitstype field like
Float64 should not allocate. Example:

julia> type A
val::Float64
end

julia> function update!(a)
for i=1:10_000_000
a.val = rand()
end
end

julia> a = A(0.0);

julia> @time update!(a)
0.048819 seconds (19.14 k allocations: 826.487 KB)

julia> @time update!(a)
0.039082 seconds (4 allocations: 160 bytes)

I don't know whether this is absolutely always true though.

—
Reply to this email directly or view it on GitHub
https://github.com/jeff-regier/Celeste.jl/pull/137/files#r55424483.

Yes, if fsm is a Vector before the assignment

fsm = (fsm[1], fsm[2])

then you'll have a type instability.

kbarbary · 2016-03-09T21:25:06Z

@rgiordan Just checking: are you going to remove SensitiveFloat.jl and make the other minor changes as part of the next PR finishing the work?

BTW git rebase can be helpful for situations like this. Rather than submitting a half-done PR to merge, run git rebase [remote]/master locally - you still have to resolve conflicts like merge, but it makes it look like your branch started from the current commit of [remote]/master, making later merging easier.

rgiordan · 2016-03-09T21:49:34Z

@kbarbary , yes, I'll fix them. Thanks for catching it. (I think I'll leave v as an array for the moment if you don't mind, though.)

Thank you for the pointer about rebasing. I am learning a lot on this project these days!

kbarbary · 2016-03-09T21:52:31Z

Yeah I agree it's not the time to fiddle with v.

…rivs Factor out bnv derivs Former-commit-id: c9efc9a

rgiordan added 11 commits March 4, 2016 16:12

Start Celeste PSF module

d8877f5

Merge branch 'master' into psf_least_squares

4de97f7

Factor out the BVN derivatives

a062516

Elbo runs with factored out derivatives

32f12c2

Partway through converting to BVN derivs type

1e581a8

Convert to BVN derivs type

f5c4486

Merge with master cleaning branch

7eb9900

Move BVN type to bivariate normals source file

c825522

Tidying up

853d938

Undo Int64 changes

a4f2dff

Fix include order

5028e03

jeff-regier added a commit that referenced this pull request Mar 8, 2016

Merge pull request #137 from jeff-regier/factor_out_bnv_derivs

c9efc9a

Factor out bnv derivs

jeff-regier merged commit c9efc9a into master Mar 8, 2016

jeff-regier deleted the factor_out_bnv_derivs branch March 8, 2016 02:15

kbarbary reviewed Mar 8, 2016
View reviewed changes

jrevels pushed a commit to jrevels/Celeste.jl that referenced this pull request Nov 3, 2016

Merge pull request jeff-regier#137 from jeff-regier/factor_out_bnv_de…

d76677c

…rivs Factor out bnv derivs Former-commit-id: c9efc9a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Factor out bnv derivs #137

Factor out bnv derivs #137

rgiordan commented Mar 7, 2016

jeff-regier commented Mar 7, 2016

kbarbary Mar 8, 2016

rgiordan Mar 8, 2016

kbarbary commented Mar 8, 2016

kbarbary Mar 8, 2016

rgiordan Mar 8, 2016

Actually a single value, but an Array to avoid memory allocation

jeff-regier Mar 8, 2016

rgiordan Mar 8, 2016

Actually a single value, but an Array to avoid memory allocation

jeff-regier Mar 8, 2016

kbarbary Mar 8, 2016

rgiordan Mar 8, 2016

Actually a single value, but an Array to avoid memory allocation

kbarbary Mar 8, 2016

kbarbary commented Mar 9, 2016

rgiordan commented Mar 9, 2016

kbarbary commented Mar 9, 2016

Factor out bnv derivs #137

Factor out bnv derivs #137

Conversation

rgiordan commented Mar 7, 2016

jeff-regier commented Mar 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbarbary commented Mar 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Actually a single value, but an Array to avoid memory allocation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Actually a single value, but an Array to avoid memory allocation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Actually a single value, but an Array to avoid memory allocation

Choose a reason for hiding this comment

kbarbary commented Mar 9, 2016

rgiordan commented Mar 9, 2016

kbarbary commented Mar 9, 2016