Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Grab-bag of cosmetic changes #134

Merged
merged 9 commits into from
Mar 5, 2016
Merged

Conversation

kbarbary
Copy link
Collaborator

@kbarbary kbarbary commented Mar 5, 2016

This PR contains no changes in functionality. Rather, these are changes to better conform to Julia conventions and changes to make the code easier to understand. I was able to clear up a few TODOs in the code along the way. The commit messages are a pretty good summary of the changes, but here's a bit more:

Minor:

  • remove @doc macro everywhere; not needed on Julia 0.4
  • Int64->Int everywhere. More correct (...if we ever ran on 32-bit) and shorter
  • use function keyword for multi-line constructors rather than begin ... end block.
  • merge benchmark_elbo.jl and benchmark_elbo_with_hessian.jl scripts; they were almost identical.

More significant:

  • Write-out ParamSet type definitions rather than using metaprogramming to define them. Only slightly longer and much more understandable.
  • Use submodules rather than putting src directory on the Julia load path. This makes it a little more obvious what namespace things are defined in.
    Files containing submodules are in CamelCase.jl whereas files that are just included into other modules are lowercase_with_underscores.jl.

@kbarbary
Copy link
Collaborator Author

kbarbary commented Mar 5, 2016

What's the deal with forward_diff_model_params()? It doesn't seem to be used anywhere in Celeste, but there's a bunch of tests on it.

@jeff-regier
Copy link
Owner

Looks good Kyle. We might want to search for dead code some time too. @rgiordan will know about forward_diff_model_params.

jeff-regier added a commit that referenced this pull request Mar 5, 2016
Grab-bag of cosmetic changes
@jeff-regier jeff-regier merged commit 4844d43 into jeff-regier:master Mar 5, 2016
@kbarbary kbarbary deleted the readability branch March 5, 2016 18:52
@rgiordan
Copy link
Contributor

rgiordan commented Mar 5, 2016

Thanks for doing all this, @kbarbary ! forward_diff_model_params is only for unit tests -- it converts a ModelParams object containing floats to one containing ForwardDiff GradientNumbers (or whatever), which allows us to text coded derivatives against autodiff derivatives.

@kbarbary kbarbary mentioned this pull request Mar 5, 2016
@kbarbary
Copy link
Collaborator Author

kbarbary commented Mar 5, 2016

Should probably move forward_diff_model_params to tests somewhere then, just to make this clear for the future.

@kbarbary kbarbary mentioned this pull request Mar 8, 2016
jrevels pushed a commit to jrevels/Celeste.jl that referenced this pull request Nov 3, 2016
Grab-bag of cosmetic changes

Former-commit-id: 4844d43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants