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

Serialization and representation GP + Small fixes #64

Merged
merged 17 commits into from
Sep 26, 2019

Conversation

stevetorr
Copy link
Contributor

Introduces a serialization method for GPs (to_ and from_dict methods) and a representation (str) method. Introduced a new string-to-kernel method in kernels.py to facilitate the from_dict method. The serialization is unit tested. Fixed a few style errors in other parts of the code. A method in gp.py sets a 'self.like' attribute and others set self.likelihood. This is also now corrected towards self.likelihood.

@stevetorr
Copy link
Contributor Author

Closes #63 , addresses #16

@stevetorr stevetorr changed the base branch from development to master September 23, 2019 16:25
Merging master onto Serialization branch
@codecov-io
Copy link

codecov-io commented Sep 23, 2019

Codecov Report

Merging #64 into master will increase coverage by 0.52%.
The diff coverage is 80.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #64      +/-   ##
==========================================
+ Coverage   46.96%   47.49%   +0.52%     
==========================================
  Files          37       37              
  Lines        5636     5740     +104     
==========================================
+ Hits         2647     2726      +79     
- Misses       2989     3014      +25
Impacted Files Coverage Δ
flare/otf.py 74.47% <ø> (ø) ⬆️
flare/gp_from_aimd.py 88.49% <0%> (-2.25%) ⬇️
tests/test_util.py 100% <100%> (ø) ⬆️
tests/test_gp.py 100% <100%> (ø) ⬆️
flare/util.py 90% <100%> (ø) ⬆️
flare/mc_simple.py 12.11% <15.38%> (+0.09%) ⬆️
flare/kernels.py 19.17% <61.53%> (+1.16%) ⬆️
flare/gp.py 86.36% <88.88%> (+0.36%) ⬆️

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 bf9bf70...790deb6. Read the comment docs.

@dmclark17 dmclark17 mentioned this pull request Sep 23, 2019
2 tasks
@dmclark17
Copy link
Contributor

Regarding the string to kernel logic, you may be able to refactor the function to use getattr and getattr. You might be able to get away with not needing to define a dictionary mapping strings to functions.

Unclear if this would be better though 🤷‍♂

@dmclark17
Copy link
Contributor

numpy.ndarrays are not JSON serializable using the default encoder. Is there a canonical way to store them on disk for the project?

(As this is mostly regarding the serialization of environments, this might not be the best place for the discussion)

@stevetorr
Copy link
Contributor Author

numpy.ndarrays are not JSON serializable using the default encoder. Is there a canonical way to store them on disk for the project?

I've been using an encoder that I found online which is included in util.py to handle converting numpy arrays to JSON.

Regarding the string to kernel logic, you may be able to refactor the function to use getattr and getattr. You might be able to get away with not needing to define a dictionary mapping strings to functions.

Thanks for looking closely at the methods! What would the getattr correspond to- getattr called on the file itself (to refer to individual callables within the flare.kernels file)? Also can you clarify what the second getattr was supposed to be?

@dmclark17
Copy link
Contributor

I see, I didn't see the NumpyEncoder 👍

Whoops that was suppose to be getattr and hasattr. You can call them on the module itself by doing something like:

current_module = sys.modules[__name__]  # Gets the current module
if hasattr(current_module, "two_body"):
    func = getattr(current_module, "two_body")

Copy link
Collaborator

@jonpvandermause jonpvandermause left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Also used this as an opportunity to address some of the linting issues that have been hanging around gp for awhile.

# self_kern = self.kernel(x_t, x_t, self.bodies, d, d, self.hyps,
# self.cutoffs)
# pred_var = self_kern - np.matmul(v_vec, v_vec)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, these lines needed to go.

self.like = like
self.like_grad = like_grad
self.likelihood = like
self.likelihood_gradient = like_grad
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, not sure why there were multiple versions of the likelihood.

for hyp, label in zip(self.hyps, self.hyp_labels):
thestr += "{}: {}\n".format(label, hyp)

return thestr
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slick.

@jonpvandermause jonpvandermause merged commit 2246c60 into master Sep 26, 2019
@jonpvandermause jonpvandermause deleted the Serialization_and_representation_GP branch September 26, 2019 14:33
@stevetorr
Copy link
Contributor Author

Thanks for taking the time to look through and provide feedback!

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.

4 participants