jeff-regier / Celeste.jl

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

Organization: moduleize bivariate normals #691

Closed kbarbary closed 7 years ago

kbarbary commented 7 years ago

This moves the part of model/bivariate_normals.jl that is model-independent (just about 2-d gaussians, not galaxies) to its own submodule BivariateNormals. This is the entire file, with the exception of GalaxyCacheComponent and BvnBundle. This will hopefully make things a little easier to understand, or make it easier to do so in the future.

I ran into a bit of a problem with the clear! function and I ended up renaming clear! to zero! as the former could be confused with the clear! in Base that has a completely different meaning. Several submodules now have their own zero! function that acts on their own types: SensitiveFloats.zero!, Model.zero!, BivariateNormals.zero! and DeterministicVI.zero!. We could make these methods of a single function Celeste.zero!, but that isn't done in this PR.

Finally, I updated a bit of code to the new where syntax while I was poking around.

jeff-regier commented 7 years ago

All 3 things seem like nice improvements. Will you merge this once the tests pass?

kbarbary commented 7 years ago

There's a travis build error on HDF5. Already tried restarting the job to no avail:

LoadError: failed process: Process(`sudo apt-get install hdf5-tools`, ProcessExited(1)) [1]
while loading /home/travis/.julia/v0.6/HDF5/deps/build.jl, in expression starting on line 39
jeff-regier commented 7 years ago

I tried restarting the build too, without luck. The weird thing is that master branch doesn't encounter build error. (I tried restarting it too.)

kbarbary commented 7 years ago

Looks like this branch ran on Travis's trusty (Ubuntu 14.04) infrastructure, whereas master ran on precise (12.04). hdf5-tools is on the apt whitelist for both precise and trusty. I think maybe the difference is that trusty defaults to sudo: false, so we might have to add a section to .travis.yml like:

addons:
  apt:
    packages:
    - hdf5-tools

I'll try that.

jeff-regier commented 7 years ago

We might want to turn on code inlining for the unit tests. The unit tests are twice as fast if inlining is allowed, but the code coverage statistics aren't as accurate. Maybe worth it for now.

kbarbary commented 7 years ago

I thought inlining was already on? The test command run on Travis,

$ julia --check-bounds=yes --color=yes -e "Pkg.test(\"${JL_PKG}\", coverage=true)"

doesn't have --inline=no. Is there some other place that inlining is turned off?

jeff-regier commented 7 years ago

Oh I didn't realize that inlining was enabled. So much for that idea. I'm not sure why these tests are taking so long.

kbarbary commented 7 years ago

Could be all the deprecation warnings on Travis. Seems like some updated dependencies have deprecations. I'll try to fix those.

jeff-regier commented 7 years ago

Still seeing depwarns in travis:

WARNING: Compat.ASCIIString is deprecated, use String instead.
  likely near /home/travis/.julia/v0.6/Celeste/test/test_galsim_benchmarks.jl:41
jeff-regier commented 7 years ago

For most of the unit tests, travis runs 3x--5x slower than my machine (when it's not computing coverage statistics). But for the galsim_benchmarks, it's more like 12x. Kind of suspicious.

giordano commented 7 years ago

Travis builds are being quite slow lately, most builds of Julia fail because of timeout. Julia developers had to ask for 30 minutes more of timeout time. This may be at least part of the reason

codecov-io commented 7 years ago

Codecov Report

Merging #691 into master will increase coverage by 4.75%. The diff coverage is 98.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #691      +/-   ##
==========================================
+ Coverage   68.83%   73.59%   +4.75%     
==========================================
  Files          37       36       -1     
  Lines        4159     3833     -326     
==========================================
- Hits         2863     2821      -42     
+ Misses       1296     1012     -284
Impacted Files Coverage Δ
src/model/log_prob.jl 89.13% <ø> (ø) :arrow_up:
src/DeterministicVI.jl 88% <ø> (ø) :arrow_up:
src/joint_infer.jl 70.52% <0%> (-14.24%) :arrow_down:
src/deterministic_vi/elbo_args.jl 100% <100%> (ø) :arrow_up:
src/deterministic_vi/elbo_objective.jl 99.42% <100%> (ø) :arrow_up:
src/BivariateNormals.jl 100% <100%> (ø)
src/SensitiveFloats.jl 93.24% <100%> (ø) :arrow_up:
src/model/fsm_util.jl 100% <100%> (ø) :arrow_up:
src/PSF.jl 95.3% <100%> (ø) :arrow_up:
... and 3 more

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 959ee05...b79a4bf. Read the comment docs.

kbarbary commented 7 years ago

The Compat.ASCIIString depwarns are most likely from FITSIO. Need to release a new version (but first fix JuliaAstro/FITSIO.jl#75).