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

MOCharges API Question? #77

Open
ryanmrichard opened this issue Oct 19, 2021 · 2 comments
Open

MOCharges API Question? #77

ryanmrichard opened this issue Oct 19, 2021 · 2 comments
Labels
good first issue Good for newcomers question Further information is requested

Comments

@ryanmrichard
Copy link
Member

ryanmrichard commented Oct 19, 2021

Right now (or maybe it's still a WIP?) MOCharges returns a number of centers by number of MOs matrix. The centers used are the ones in from space of the MOs. This could conceivably not be what the caller wants because: e.g., the same center was added to the AO basis set multiple times, there are ghost atoms, and/or there are mid-bond functions. My proposal is:

  • we introduce a class PointSet which is a container of Point objects (this would live in LibChemist)
  • Refactor the Atom class to inherit from PointCharge (related to this issue)
  • Ensure AOBasisSet and Molecule are implicitly convertible to PointSet
  • Have MOCharges take a PointSet instance and a DerivedSpace instance

This allows the caller of MOCharges to specify the centers they care about.

Thoughts? Takers?

Edit: Also I propose the name MOPopulations since AFAIK this is simply counting the number of electrons at a point (the electronic charge would -2 times the matrix returned by this PT for restricted spatial orbitals).

@ryanmrichard ryanmrichard added good first issue Good for newcomers question Further information is requested labels Oct 19, 2021
@jwaldrop107
Copy link
Member

The structure of the return tensor for these modules is the intended form, but the name MOCharges is probably not the most appropriate one. I think all of this makes sense. I can change the name on the property type whenever. I can also work on the other things here, but probably not at this moment unless it is particularly pressing.

@ryanmrichard
Copy link
Member Author

It's not immediately pressing because I use hard-coded Mulliken populations for testing the sparse map formation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants