jeff-regier / Celeste.jl

Scalable inference for a generative model of astronomical images
MIT License
183 stars 28 forks source link

Remove multiple includes of bivariate_normals code #689

Closed kbarbary closed 7 years ago

kbarbary commented 7 years ago

This is simply code organization, removing the multiple includes of bivariate_normals.jl. Instead, the code now lives solely in the Model submodule and is imported into other submodules as needed (PSF and Synthetic.

~I also split the bivariate_normals.jl file into two files:~

codecov-io commented 7 years ago

Codecov Report

Merging #689 into master will not change coverage. The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #689   +/-   ##
=======================================
  Coverage   68.83%   68.83%           
=======================================
  Files          37       37           
  Lines        4159     4159           
=======================================
  Hits         2863     2863           
  Misses       1296     1296
Impacted Files Coverage Δ
src/PSF.jl 95.3% <ø> (ø) :arrow_up:
src/model/bivariate_normals.jl 100% <100%> (ø)
src/Synthetic.jl 74.57% <100%> (ø) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8d43030...e02ac6d. Read the comment docs.

jeff-regier commented 7 years ago

I'm pretty sure we'll want to keep the bivariate normal derivative code together with the code that's being differentiated: the two are very tightly coupled. Even the slightest change to the code means the derivatives need to change. Is there some other way to achieve what you're going for in this PR?

kbarbary commented 7 years ago

Assuming you're referring to splitting bivariate_normals.jl and derivatives.jl, absolutely I can put them back in the same file. (They're currently included one after the other, so this is only a matter of organization.)

It wasn't apparent from reading the code that the stuff in the first file depended on anything in the second, and I just thought that a separation between "generic math stuff" and "model-specific stuff that uses the generic math stuff" might be useful for humans.

kbarbary commented 7 years ago

Removed split.

jeff-regier commented 7 years ago

Some of the variable names in that file unfortunately suggest that code is specific to the model (e.g. galaxies). In fact it's all better thought of as "generic math stuff", though you wouldn't know it from the variable names.