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

replace linear regression by ridge regression #933

Merged
merged 5 commits into from
Dec 11, 2023
Merged

replace linear regression by ridge regression #933

merged 5 commits into from
Dec 11, 2023

Conversation

samwaseda
Copy link
Member

No description provided.

@samwaseda samwaseda requested a review from pmrv December 7, 2023 21:31
Copy link
Contributor

github-actions bot commented Dec 7, 2023

Binder 👈 Launch a binder notebook on branch pyiron/pyiron_contrib/correct_qha

@coveralls
Copy link

coveralls commented Dec 7, 2023

Pull Request Test Coverage Report for Build 7145442051

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 141 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.006%) to 16.368%

Files with Coverage Reduction New Missed Lines %
atomistics/atomistics/master/qha.py 141 0.0%
Totals Coverage Status
Change from base Build 7088081052: -0.006%
Covered Lines: 2524
Relevant Lines: 15420

💛 - Coveralls

Copy link
Contributor

@pmrv pmrv left a comment

Choose a reason for hiding this comment

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

Wouldn't the cross validation or the regularization push the fit to not consider directions of low force for the fitted displacements?

I assume you put this because sometimes the linear regression found unrealistically large displacements because of some noise? Could you check whether the fitted displacements match the original ones to catch that instead?

EDIT: Not sure if this really has an influence, just guessing where you are coming from.

@samwaseda
Copy link
Member Author

I assume you put this because sometimes the linear regression found unrealistically large displacements because of some noise? Could you check whether the fitted displacements match the original ones to catch that instead?

Your assumption is correct, but this fitting has nothing to do with the Hessian matrix fitting. This is the process to make sure that the displacements will be defined with respect to the correct energy minimum, because depending on the initial system the user is giving the energy is given by:

$$U = U_0 + \frac{1}{2} (x-x_0)^T \mathcal{H} (x-x_0)$$

And this fitting delivers $x_0$. In principle, if the second order fit (i.e. when the displacement is both in the positive and negative direction) is performed, there will be one more vector than needed, because I do also a calculation without displacements, so purely mathematically speaking ridge regression should work. Even if it doesn't, it only means $x_0$ would be incorrectly set to 0, so it wouldn't lead to a dramatic result.

@samwaseda
Copy link
Member Author

But I guess ideally the user should have the possibility to set this part more flexibly.

@pmrv
Copy link
Contributor

pmrv commented Dec 7, 2023

Ok, that I understand then, but couldn't you just np.allclose(reg.predict(self.forces), self.displacements) to see if the fit is good and x0 is reasonable?

@samwaseda
Copy link
Member Author

That's a great idea. I just added a check.

@pmrv
Copy link
Contributor

pmrv commented Dec 7, 2023

Ah, probably reg.coef_ is actually what should be compared to self.displacements, but any check of the fit quality will do, so works for me.

@samwaseda
Copy link
Member Author

Ah, probably reg.coef_ is actually what should be compared to self.displacements, but any check of the fit quality will do, so works for me.

No still not :D reg.coef_ is the vector showing the direction of the predicted energy minimum positions.

@pmrv
Copy link
Contributor

pmrv commented Dec 8, 2023

Anyway, did this have a big impact on the final free energies? I can almost not imagine it, because I think at the least the structures I had tested it on should have been close to equilibrium already.

@samwaseda
Copy link
Member Author

Somehow now it looks like I managed to fix it. Don’t ask me how XD I think I have to test it a bit more in contrib before I can be confident that it can move elsewhere.

@pmrv
Copy link
Contributor

pmrv commented Dec 8, 2023

Mh, strange, let me test again also next week. It'd be a bit weird, because I thought the structures I had tested it on previously were already relaxed, so I'd be surprised if the changes here make a big difference.

@samwaseda
Copy link
Member Author

as far as I can see now phonopy and my class deliver the same heat capacity

@samwaseda
Copy link
Member Author

After today's discussion I got the feeling that this class is no longer needed. Since I know that this one solves the one problem that we talked about, I merge it, but it will probably not see more development in the future.

@samwaseda samwaseda merged commit 4ca8fc9 into main Dec 11, 2023
15 of 16 checks passed
@samwaseda samwaseda deleted the correct_qha branch December 11, 2023 15:40
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.

3 participants