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

Clean up CalibrateEmulateSample #67

Merged
merged 3 commits into from
Jul 23, 2020
Merged

Clean up CalibrateEmulateSample #67

merged 3 commits into from
Jul 23, 2020

Conversation

bielim
Copy link
Contributor

@bielim bielim commented Jul 22, 2020

This PR is a major clean-up (goal: code hygiene and reduction of the insanely long precompile times, i.e., addressing #65) -- the following changes were made:

(1) Deleted the modules EKS.jl, GPR.jl, Histograms.jl, problems.jl, and spaces.jl. These modules are not functional and not being developed anymore.

(2) Deleted test/EKS, test/Histograms, test/GPR, test/L96m, as well as examples/GPR and examples/L96, because they are based on non-functional modules or (in the case of L96) because they don't seem to demonstrate any CES functionality.

(3) Deleted test/Cloudy, moved and renamed its content (test/Cloudy/runtests.jl) to examples/Cloudy/Cloudy_example.jl. The Cloudy "test" never was a proper test but rather an example, so it fits much better in the examples folder. The Cloudy notebook (examples/Cloudy/calibrate_emulate_sample_Cloudy_demo.ipynb) was deleted as it's basically just a more "eye-friendly" version of Cloudy_example.jl.

(4) Moved GModel.jl to examples/Cloudy. This module shows what a forward model G that maps the parameters u to G(u) could look like, but it does so using Cloudy as an example, and for that reason I never liked the fact that this module was there with all the general-purpose code in the first place. As a result of (3) and (4), there is no more Cloudy dependency in the tests or core CES code.

(5) Generated separate Project.toml and Manifest.toml files for each example. With this setup, running the core CES code does not require to precompile packages that are only used in the examples (e.g., DifferentialEquations.jl) anymore.

(6) Removed unused packages from the main Project.toml file

Finally, I also changed the way the ScikitLearn.jl imports are handled in GPEmulator.jl. At least on my machine, this solves the problems with the seg faults that I mentioned in the discussion to PR #63.

@bielim bielim self-assigned this Jul 22, 2020
@bielim bielim added the cleanup Code clean-up and/or reorganization label Jul 22, 2020
@bielim
Copy link
Contributor Author

bielim commented Jul 22, 2020

@dburov190: I believe you are the creator of L96 -- I can see that it is an implementation of the Lorenz 96 model, but I'm not sure if it demonstrates any CES functionality. If it does, please let me know, and I'll re-include it. If not, I'd suggest to find another place/repo for it, in the interest of keeping the CES repo as clean as possible.

@bielim
Copy link
Contributor Author

bielim commented Jul 22, 2020

@charleskawczynski: One issue I see with the current structure (separate Project.toml files for each example while keeping only the packages needed for core CES functionality in the main Project.toml) is that it is very easy to accidentally add some of the example dependencies back into the main Project.toml: E.g., when I am inCalibrateEmulateSample.jl/ and do include("examples/Cloudy/Cloudy_example.jl") (rather than running the example from within examples/Cloudy), it will automatically start to update the main Project.toml with the packages needed to run Cloudy_example.jl. Is there a way to keep this from happening?

…ject.toml, this removed the chaff from the Manifest
bors bot added a commit that referenced this pull request Jul 23, 2020
@bors
Copy link
Contributor

bors bot commented Jul 23, 2020

try

Build succeeded:

@bielim
Copy link
Contributor Author

bielim commented Jul 23, 2020

bors r+

@CliMA CliMA deleted a comment from bors bot Jul 23, 2020
@bors
Copy link
Contributor

bors bot commented Jul 23, 2020

Build succeeded:

@bors bors bot merged commit 0f3bb93 into master Jul 23, 2020
@bors bors bot deleted the mb/spring_cleaning branch July 23, 2020 23:21
@jakebolewski
Copy link
Contributor

35 -> 13 minutes, nice!

@charleskawczynski
Copy link
Member

@bielim looks like you figured out how to avoid this auto-project update problem. Please reach out if this is not the case!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Code clean-up and/or reorganization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants