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

Basis set tutorial rebased #158

Merged
merged 16 commits into from
Jan 18, 2024

Conversation

marco-2023
Copy link
Contributor

This is #145 with revisions and rebased on master (so the examples run without problem).

This PR goes through the examples of making basis set instances and the different ways to do so.

Checklist

  • 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

Type of Changes

Type
📜 Docs

Related

maximilianvz and others added 11 commits January 12, 2024 02:14
Done:
- The notebook was restructured (for better reading).
- The transformations section was removed (fits better with other tutorial)

Notes:
- The outputs seem very repetitive
- Some of the code block may be better with less outputs
@marco-2023 marco-2023 mentioned this pull request Jan 12, 2024
5 tasks
@FarnazH FarnazH self-requested a review January 12, 2024 16:53
@FarnazH
Copy link
Member

FarnazH commented Jan 12, 2024

@marco-2023, alternatively, you could have force-pushed to @maximilianvz's branch. Does this mean that PR #145 can be closed? I will review this PR soon.

@marco-2023
Copy link
Contributor Author

@FarnazH @maximilianvz I went through the notebook one last time and I found errors in the last two sections (IODATA and pySCF) I fixed them and I also changed some variable names to distinguish between a basis and a shell. Sorry about that.

@maximilianvz
Copy link
Collaborator

maximilianvz commented Jan 12, 2024

@marco-2023, could you please add me as a collaborator to your fork of gbasis? I have some minor formatting changes to push to this branch.

Separately, I'm going to ask you to take a look at section 1.2.3 again. The highlighted section of the text here is a little unclear to me:

Screenshot 2024-01-12 at 5 56 15 PM

In this case, when you pass a list of coord_types to make_contractions, if hydrogen 1 contributes 3 shells to basis and hydrogen 2 contributes 3 shells to basis, the coord_types list should be of the form [coord type for first shell of hydrogen 1, coord_type for second shell of hydrogen 1, coord_type for third shell of hydrogen 1, coord type for first shell of hydrogen 2, coord_type for second shell of hydrogen 2, coord_type for third shell of hydrogen 2] (the first 3 coordinate types correspond to hydrogen 1 shells and the last 3 to hydrogen 2 shells).

In this example, did you intend to have the shells for hydrogen 1 all in Cartesian coordinates and all the shells for hydrogen 2 in spherical coordinates? If so, the list should be something like ['c', 'c', 'c', 'p', 'p', 'p'] (or equivalent).

@FarnazH
Copy link
Member

FarnazH commented Jan 12, 2024

Thanks @marco-2023, I went through the notebook and made some changes. I will share it tomorrow after finishing sketching the corresponding section of the paper.

@marco-2023
Copy link
Contributor Author

Hi @maximilianvz thanks for pointing this out. At the beginning that was my idea but this later changed to depicting how you could arbitrarily set the types of contracted shell types (I forgot to update the markdown text). Can you please fix this?

PD: If you think is better to use cartesian shells for one atom and spherical for the other please feel free make the changes.

@maximilianvz
Copy link
Collaborator

@marco-2023, I think it would be clearer to point out to users that the length and ordering of the coord_types list matters with respect to the specified list of atoms. I'll change it to Cartesian for hydrogen 1 and spherical for hydrogen 2, and clarify this in the highlighted text from the screenshot I sent.

@maximilianvz
Copy link
Collaborator

I just updated the example as per my last comment. @FarnazH or @marco-2023, I ask that you inspect my changes and ensure that I haven't made the wording above the code cell in Section 1.2.3 too lengthy. Thanks.

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.

I rebased on my earlier changes and pushed it. However, if I have inadvertently removed any of your fixes, please add it back and make a new PR. Thanks for all your work @maximilianvz and @marco-2023.

@FarnazH FarnazH merged commit 97713cd into theochem:master Jan 18, 2024
0 of 9 checks passed
@FarnazH
Copy link
Member

FarnazH commented Jan 18, 2024

@marco-2023 and @maximilianvz: some changes were missing resulting in an invalid notebook, so I just pushed some fixes to master; see 97713cd. For the latest notebook see: https://github.com/theochem/gbasis/blob/master/notebooks/tutorial/Tutorial%201%20-%20Basis%20Sets.ipynb

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