Conversation
|
This should probably be added as a separate release, it is still in alpha but I said in a paper I would make this code available and it has almost been a year.... I know, it's shameful |
Added links to CI and PES-Learn paper DOI
|
Updating the README so the DOI of the original PES-Learn paper is included with link. And updating the CI link. |
| """ | ||
| self.hyperparameter_space = { | ||
| 'scale_X': hp.choice('scale_X', ['std', 'mm01', 'mm11', None]), | ||
| #'scale_X': hp.choice('scale_X', ['std', 'mm01', 'mm11', None]), |
There was a problem hiding this comment.
Is there a reason to get rid of hp choice for scaling X data?
There was a problem hiding this comment.
This looks like a remnant of me messing around with GPR. Should be reverted to original.
| self.ytest = self.y[self.test_indices] | ||
|
|
||
| def build_model(self, params, nrestarts=10, maxit=1000, seed=0): | ||
| params['scale_X'] = 'std' |
There was a problem hiding this comment.
Same thing here, why set scale_X to always be 'std'?
| print("Max 5 errors: {}".format(np.sort(np.round(max_errors.flatten(),1))),'\n') | ||
| error_test_invcm = round(hartree2cm * error_test,2) | ||
| return error_test_invcm | ||
| return error_test |
There was a problem hiding this comment.
All errors currently are reported in Hartrees, this would change this one error to be reported in cm^-1. I am open to discussion on making this change, but it would need to be consistent across PES-Learn.
| g = np.apply_along_axis(cart1d_to_distances1d, axis, g) | ||
| newX = gp.transform_new_X(g, params, Xscaler) | ||
| E, cov = final.predict(newX, full_cov=False) | ||
| E, cov = model.predict_f_compiled(newX) |
There was a problem hiding this comment.
What is predict_f_compiled() function? I don't see it in the GPy documentation.
There was a problem hiding this comment.
This is probably from GPyTorch or GPFlow. Once upon a time I was migrating PES-Learn to those Gaussian Process libraries and this looks like a missed remnant.
| # data = pd.read_csv(path, sep=None) | ||
| return data | ||
|
|
||
| @abstractmethod |
There was a problem hiding this comment.
These @abstractmethods are meant to be here to enforce structure within ML model classes. I see that not all of the mfnn methods have these methods, could you please return the @abstractmethods here and add build_model, save_model, preprocess and split_train_test to all mfnn methods, even if they are blank functions and just pass.
There was a problem hiding this comment.
All of these issues come from when I was messing around with the PES-Learn code. I thought when I made this PR that none of that was included. Clearly I was mistaken. I'll restart from a fresh PES-Learn branch.
|
Making new PR with current PES-Learn and my stuff added in. |
2 participants
Added support for mutlifidelity models as outlined in https://doi.org/10.1063/5.0158919.