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

QC model method contributions #160

Draft
wants to merge 39 commits into
base: develop
Choose a base branch
from
Draft

Conversation

EBB2675
Copy link
Collaborator

@EBB2675 EBB2675 commented Dec 16, 2024

The first part of ModelMethod contributions for chemistry.

Up until now:

  • HartreeFock

  • PerturbationMethod

  • MolecularOrbital

  • LCAO

  • LocalCorrelation

  • AtomCenteredBasisSet

  • GTOIntegralDecomposition

  • NumericalIntegration

  • OrbitalLocalization

Copy link
Collaborator

@JFRudzinski JFRudzinski left a comment

Choose a reason for hiding this comment

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

Looks very nice! I only gave some minor formatting suggestions.

To fully assess the schema, it would help to see the parser alongside or even better an example upload that I can use the metainfo browser to inspect.

@ndaelman-hu should of course approve before you merge.

functional_composition = SubSection(
sub_section=AtomCenteredFunction.m_def, repeats=True
) # TODO change name
)

def normalize(self, archive: 'EntryArchive', logger: 'BoundLogger') -> None:
super().normalize(archive, logger)
# self.name = self.m_def.name
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't forget to do something with this.

Do we already have some sort of convention the names? It seems like most or at least a lot of classes have the line you are commenting out here, but I have also seen self.name = self.m_def.name if self.name is None else self.name which allows the parser to overwrite this name.


energy = Quantity(
type=np.float64,
# unit='electron_volt',
Copy link
Collaborator

Choose a reason for hiding this comment

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

no unit?

- canonical: Default canonical molecular orbitals.
- natural: Natural orbitals obtained from the density matrix.
- localized: Localized orbitals such as Boys or Foster-Boys localization.
TODO: this will be later connected to many other things.
Copy link
Collaborator

Choose a reason for hiding this comment

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

move TODO outside of description

Copy link
Collaborator

Choose a reason for hiding this comment

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

and be a bit more explicit to what you'd like to connect

type=np.float64,
description="""
Total spin of the system defined as S = (n_alpha_electrons - n_beta_electrons) / 2.
Connect to the model system.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a TODO? Clarify or move out of description

f'For {self.reference_type}, the number of alpha and beta electrons must be equal.'
)
return False
if self.reference_type in ['ROHF', 'ROKS']:
Copy link
Collaborator

Choose a reason for hiding this comment

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

elif?

density = Quantity(
type=MEnum('relaxed', 'unrelaxed'),
description="""
unrelaxed density: MP2 expectation value density
Copy link
Collaborator

Choose a reason for hiding this comment

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

Capitals and periods, or make a table as above

- DLPNO-CCSD(T): Domain-Based Local Pair Natural Orbital CCSD(T)
- LocalDFT: Local Density Functional Theory.

# TODO: improve list!
Copy link
Collaborator

Choose a reason for hiding this comment

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

move TODO out of description

""",
)

integration_thresh = Quantity(
Copy link
Collaborator

Choose a reason for hiding this comment

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

rename to "integration_threshold" (if there are other quantities named "thresh" throughout the schema, I would also like to rename those), maybe add a TODO for this

orbital_window = Quantity(
shape=['*'],
description="""
the Molecular orbital range to be localized.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Capital "The"

core_threshold = Quantity(
type=np.float64,
description="""
the energy window for the first occupied MO to be localized (in a.u.).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Capital "The"

Copy link
Collaborator

@ndaelman-hu ndaelman-hu left a comment

Choose a reason for hiding this comment

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

Covered model_method and mode_system now. Will do the rest in a future review.
You can resolve these comments in the meantime.

Comment on lines 1241 to 1243
symmetry_label = Quantity(
type=str, description='Symmetry label of the molecular orbital.'
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

consider type=MEnum, so you can better distinguish with the atoms state

A section representing a single molecular orbital.
"""

energy = Quantity(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is also covered by ElectronicEigenvalues, which lists the values and occupations.
Is the energy meant here to simply identify the state?

A base section for Hartree Fock ansatz.
"""

reference_determinant = Quantity(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe you also want to store a reference to the matching DFT calculation?
Then you could even extract this label with normalization.


# TODO: improve list!
""",
a_eln=ELNAnnotation(component='EnumEditQuantity'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that this does not yet provide any logic for saving the user input.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The methodologies are fine, just minor comments.

I'd introduce a single ElectronicState instead of OrbitalState or MolecularOrbital...
Don't get me wrong, these may keep on existing (for now), but they should specialize ElectronicState via inheritance. This can be covered in a follow-up PR, but pls bear this in mind for your current approach.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thx for adding these quantities. I take it that the actual normalization will be in a follow-up PR?

method = Quantity(
type=MEnum('LMP2', 'LCCD', 'LCCSD', 'LCCSD(T)', 'DLPNO-CCSD(T)', 'LocalDFT'),
description="""
The local correlation method applied. For example:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pls elaborate a bit more here. Feel free to also add references for specific methods.

Comment on lines 1410 to 1411
def normalize(self, archive: 'EntryArchive', logger: 'BoundLogger') -> None:
super().normalize(archive, logger)
Copy link
Collaborator

@ndaelman-hu ndaelman-hu Dec 20, 2024

Choose a reason for hiding this comment

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

The normalization is redundant here

| `'BW'` | Brillouin-Wigner |
""",
a_eln=ELNAnnotation(component='EnumEditQuantity'),
) # TODO: check if the special symbols are supported
Copy link
Collaborator

Choose a reason for hiding this comment

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

Latex is indeed supported

Copy link
Collaborator

Choose a reason for hiding this comment

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

Your changes to Mesh only affect the quantity names and descriptions, correct?
This is fine, but in general I'd leave this for a separate, quick PR. Now we have to check whether all other places that use this interface have been updated too.

The new sections I'll cover in a future review.

@coveralls
Copy link

coveralls commented Jan 2, 2025

Pull Request Test Coverage Report for Build 12788768540

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-80.1%) to 0.0%

Totals Coverage Status
Change from base Build 12272374018: -80.1%
Covered Lines: 0
Relevant Lines: 0

💛 - Coveralls

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