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

Add Quickstart notebook #202

Merged
merged 11 commits into from
Dec 24, 2023
Merged

Conversation

marco-2023
Copy link
Collaborator

No description provided.

Copy link
Member

@FarnazH FarnazH left a comment

Choose a reason for hiding this comment

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

@marco-2023, thanks for your PR. The content and logic of your notebook were great; I working through it. I added my revisions in b734bbc. Some of the changes were made automatically by my editor, and the changes I made addressed (the list is not complete):

  1. The links should be to the webpage not the rst files in the repo. Unless I am missing sth, your links didn't work for me.
  2. When making an instance of a class like MolGrid, it would be good to showcase its attributes by printing some information about the grid. I changed the print statements to do that.
  3. It is better for MolGrid example to show an example of a molecule (instead of H atom, which is useful but can be confusing as a quick start example). Then, we don't need to remake the MolGrid for H2+ when using the grid for computations.
  4. Sometimes the markdown description did not match the code block, so I updated that. For example, Formaldehyde was mentioned in the comments when H2+ was used in the code/markdown.
  5. Let's keep our variable names consistent across notebooks and packages. For example, we have been using rgrid, atgrid, and oned (probably we should make a glossary). It is important to stay consistent for easier understanding and use.
  6. Sometimes the text in the markdown and comments repeated the same thing. I would keep the comments minimal, so that code is simpler and easier to maintain. I updated the notebook to fix these.
  7. I added a small section to check the accuracy of interpolated density against the analytical value.
  8. Moved Cubic and Periodic grids to the end of the notebook to move using the grid for computation (integration and interpolation closer to MolGrid section).

After @Ali-Tehrani has reviewed the notebook, we can merge it, and you can make a new PR when the cubic/periodic parts are done.

This was referenced Nov 18, 2023
@marco-2023
Copy link
Collaborator Author

@FarnazH Thanks for the review and the comments. The guidelines you shared will help me a lot in the upcoming notebooks!

I have a doubt regarding the first point. I made the links pointing to the corresponding rst files following what was previously done in the Molecular grid notebook. The ideas is that the links are generated by sphinx when building the documentation. When I did it on my computer worked. Should we change and use the static links instead?

marco-2023 and others added 7 commits December 11, 2023 10:51
An example was added for calculating, visualizing and
integrating the lobes of formaldehyde dual descriptor.

Notes
The example is a little bit long and perhaps even complicated.
The fchk files necessary for calculating formaldehyde dual descriptor
are added.
1. The cubic grid section showcased a great example, but was too elaborate and
   complicated to be suitable for a short quickstart.
   I believe the Quickstart.ipynb does not need to cover all the functionality,
   so it is best to add this example to the notebook dedicated to Cubic grid.
2. Add a section for differentiation on the grid.
@FarnazH FarnazH merged commit 044ca85 into theochem:master Dec 24, 2023
6 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.

2 participants