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

Evaluations Tutorial Notebook #146

Merged
merged 22 commits into from
Apr 29, 2024

Conversation

maximilianvz
Copy link
Collaborator

Steps

  • Write a good description of what the PR does.
  • Add tests for each unit of code added (e.g. function, class)
  • Update documentation
  • Squash commits that can be grouped together
  • Rebase onto master

Description

This is a separated notebook corresponding to the Evaluations section of the notebook from PR #142 (closed).

Type of Changes

Type
🐛 Bug fix
✨ New feature
🔨 Refactoring
📜 Docs

Related Issue

@maximilianvz
Copy link
Collaborator Author

In this PR and #147, I have just updated the template examples to make more sense (i.e., dimension of MO basis is same as dimension of AO basis and only one density matrix is used). Again, this is just a template, so we need a better example. Perhaps @PaulWAyers should weigh in here (you mentioned hydrogen selenide as a good running example, I believe).

@PaulWAyers
Copy link
Member

PaulWAyers commented Jan 12, 2024

If we want an example where we have no calculations undergirding it, the ANOs are a good choice. The ANOs themselves are "molecular orbitals" and if one wishes to check a transformation thereof, one can do a random transformation with unit determinant, i.e., a transformation in $sl(n,\mathbb{R})$, by choosing constructing two random nxn matrices, $\mathbf{A}$ and $\mathbf{B}$ and then making the transformation matrix:

$$ \mathbf{T} = \exp \left(\mathbf{A} - \mathbf{A}^T \right) \cdot \exp \left(\mathbf{B} + \mathbf{B}^T - 2 \text{diag}(\mathbf{B})\right) $$

where the exponentiation is the matrix exponential and $\text{diag}(\mathbf{B})$ is a diagonal matrix containing the diagonal elements of $\mathbf{B}$. One could then convert the MOs to AOs using the transformation $\mathbf{T}$ and the AOs back to MOs with the inverse transformation defined by the matrix inverse, $\mathbf{T}^{-1}$.

This would have the "right structure" for AOs (nonorthogonal but normalized and linearly independent) and MOs (orthogonal and normalized).

As for a "real molecule" I think that $\ce{H2Se}$ was an interesting example from the Paris workshop, and I think there would be an example *.fchk already in the databases repository.

@marco-2023
Copy link
Contributor

marco-2023 commented Jan 15, 2024

Hi @maximilianvz and @FarnazH, I am very sorry for the lateness, I know we are short on time. Here is the evaluation tutorial (the part regarding the electron density and associated properties). I split it into parts (if not it would be too big). I think some of these examples can be used for the manuscript (at least the ideas). If needed I can make a version with the graphs with better quality. I used the formaldehyde molecule but we can (easily) change it. I will be working on what is missing, evaluations of orbitals, properties derived from 1rdm and electrostatic potential.

PD: The headings across the notebooks have an inconsistency. In Tutorial 1 - Basis Sets.ipynb and Tutorial 3 - Integrals.ipynb, they are numbered, in Tutorial 2 - Evaluations.ipynb not. We should decide on one scheme. Do you have any suggestions? I also think the file names should be more illustrative.

@maximilianvz
Copy link
Collaborator Author

@marco-2023, thanks for your updates to this notebook. The added visual aspects look really great! Based on my understanding of your message above, you want to make separate notebooks for density/related properties, electrostatic potential, etc. Is that correct?

My main comment is on the last part of the notebook:

Screenshot 2024-01-15 at 3 15 46 PM

What is promised in the highlighted text is not done. I'm not sure if you plan on adding a plot for this part of the notebook, but if not, that sentence should be removed. Also, the part about "In the example code below, presume the following derivative of the density is desired:" should be removed, I believe.

On the topic of numbering versus not numbering headings, it seems like it's easier to get away with not numbering them in this notebook as compared to the others, because each section is pretty intimately connected with the last. In this way, you can tell a story nicely about what you're doing, but I don't see why you couldn't still number the headings. In my opinion, its probably easier to keep numbering them, but if feel strongly about it, I have no problem with removing them from the other notebooks (this is probably the option requiring more work), so long as fluidity is not compromised.

I agree that the file names could be more illustrative. If you have suggestions, please push them. Thanks!

@marco-2023
Copy link
Contributor

Thanks, @maximilianvz for going through the notebooks in detail. I fixed the issues you pointed out. It happened again that I first wrote the idea that was going to be implemented, then changed it to something more interesting and then I forgot to update the markdown part. Sorry for that.

@marco-2023
Copy link
Contributor

@maximilianvz, @FarnazH I added the last examples notebook (it took me more than I thought, sorry about that). The order of the notebooks can be improved (I did not have as much time as I would want for this).

@maximilianvz, feel free to modify them.

  • I did not use numbers on the headings because I was not clear yet on the order (I am not a huge fan of using numbers, if anything is added later the numbering needs fixing, but I leave it to your discretion).
  • If you can, go through them and see if he 'flow' can be improved

@FarnazH when you can, please let me know which parts will be used on the paper and I can make the paper notebook .

Again sorry for the lateness.

@maximilianvz
Copy link
Collaborator Author

@marco-2023, it looks to me like there are two nearly identical notebooks here: Evaluations_electron_density.ipynb and Electron_density_and_gradient.ipynb. Is there a reason for this? If I'm not mistaken, one of these is redundant, so please let us know which one should be kept.

@maximilianvz
Copy link
Collaborator Author

@marco-2023, in Evaluations_1rdm_properties.ipynb, you give an example showing calculations of positive definite kinetic energy density, but this is also done in Evaluations_electron_density.ipynb. I'm not sure we need to show this in both tutorials.

On the topic of Ehrenfest Hessian from Evaluations_1rdm_properties.ipynb, which you know about more than me, is this indeed supposed to evaluate to zero at the centre of mass? In the last example in the notebook, you have lines like

Screenshot 2024-01-18 at 4 39 42 PM

where you say that it should be zero, but the assertion yields False. If you didn't make any typos here, then it seems like there's a problem with the example, because the Ehrenfest Hessian isn't zero at the centre of mass. I'd appreciate your clarification here.

@marco-2023
Copy link
Contributor

Hi @maximilianvz I renamed the file Evaluations_electron_density.ipynb to Electron_density_and_gradient.ipynb using git mv. In principle, the file Evaluations_electron_density.ipynb should not exist anymore so don't take it into account. The file that should be revised is Electron_density_and_gradient.ipynb. Aparently I messed up somewhere. Do you want me to push a commit deleting it or you want to do it?

On this note, this second file does not have the positive definite kinetic energy example, I moved the example to 1rdms properties because it is a property derived from it.

@PaulWAyers
Copy link
Member

I think Ehrenfest force should be zero (a zero vector) at the center-of-mass for a symmetric system, as it will be a critical/Lagrange point. I would be very surprised if the Ehrenfest Hessian were the zero matrix.

@marco-2023
Copy link
Contributor

Hi @maximilianvz , I fixed the problem of the duplicated notebooks. I left Evaluations_electron_density.ipynb because I was the one you revised earlier.

@marco-2023
Copy link
Contributor

Thank you @PaulWAyers for pointing out the error, I fixed the print statement accordingly.

@leila-pujal
Copy link
Collaborator

Hi @marco-2023, I was checking this tutorials, I might be wrong but I think you may be referencing the wrong fchk in the Evaluations_1rdm_properties.ipynb? You are trying to load c2h4_q0.fchk but in the data folder I only see ch2o_q_0.fchk. Let me know if I am doing something wrong.
image

@marco-2023
Copy link
Contributor

Hi @leila-pujal
You are absolutely right. If I remember correctly at the beginning the tutorial used the formaldehyde as an example. Latter we changed to $\rm C_2H_4$ because it is a symmetric system. And I forgot to add the fchk to git. Sorry about that. I will add it now. I think we can also remove 398eace .
Thank you for noticing!

@leila-pujal
Copy link
Collaborator

No worries! Thanks for making the tutorials they look really good with the visualization. I'll message you again if I have other doubts before merging the PR and the integrals one. Thanks again for all the work you put into making the tutorials.

@leila-pujal
Copy link
Collaborator

Hi @marco-2023 and @maximilianvz , I went over your notebooks a while ago and everything looked good to me. I am adding some minor changes, mainly to what fchk files we are adding to the repo. I force-pushed this branch because pre-commit bot made changes before we excluded this folder from its workflow. I will merge this after the tests pass. Thanks a lot for working on this.

@leila-pujal leila-pujal merged commit 643f898 into theochem:master Apr 29, 2024
10 of 11 checks passed
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