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

Extend screening overlap to IOData wrapper #171

Merged
merged 4 commits into from
Apr 16, 2024

Conversation

leila-pujal
Copy link
Collaborator

@leila-pujal leila-pujal commented Apr 2, 2024

This PR builds on top of PR #111 to extend the screening overlap functionality to data load from and IOData object. PR #111 implemented the screening of overlap type integrals. This feature is set up at the user level when the function make_contractions is employed in combination with a parsed basis from a Gaussian or NWChem basis set. The same API is added to the function from_iodata to be able to access the overlap screening feature. test_wrappers.py has been updated and also a new test has been added to check the new API correctly stores the overlap screening information passed when instantiating each GeneralizedContractionShell

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
🐛 Bug fix
✨ New feature

Related

"""Return basis set stored within the `IOData` instance in `iodata`.

Parameters
----------
mol : iodata.iodata.IOData
`IOData` instance from `iodata` module.
tol : float
Tolerance used in overlap screening.
ovrlap : bool
Copy link
Collaborator

@maximilianvz maximilianvz Apr 15, 2024

Choose a reason for hiding this comment

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

Typo to fix here ("ovrlap" should be "overlap").

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

@maximilianvz
Copy link
Collaborator

@leila-pujal, this looks good to me. The only thing I might suggest is updating the documentation of the __init__ function of the GeneralizedContractionShell class so that the argument names match what is written. Specifically, in this line, the argument tol (given a default value of 1e-15) is referenced in the documentation here as ovr_tol. This caused a slight bit of confusion for me, because when instantiating an IOData shell object as you did here or when instantiating a GeneralizedBasisContraction object as is done in make_contractions here, you obviously need to pass tol=tol rather than ovr_tol=tol (otherwise, the code will break). When I was trying to cross-reference with the documentation, this difference in naming caught me off guard. You could combine this in a commit with a correction of the typo I pointed out above.

@leila-pujal
Copy link
Collaborator Author

leila-pujal commented Apr 15, 2024

Thanks @maximilianvz for your detailed and quick review. I overlooked that typo (probably because in some other places the abbreviation is used). You are right about the mismatch in the documentation. I address it in my last commit as well as the typo (like you suggested). I've asked for another revision from you. Let me know if I forgot to address something from your review.

Copy link
Collaborator

@maximilianvz maximilianvz left a comment

Choose a reason for hiding this comment

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

Looks good to me now, @leila-pujal.

@leila-pujal leila-pujal merged commit 32a40f4 into theochem:master Apr 16, 2024
0 of 9 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