Conversation
…ction coefficients
…ined orbital types
JFRudzinski
left a comment
There was a problem hiding this comment.
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.
|
|
||
| def normalize(self, archive: 'EntryArchive', logger: 'BoundLogger') -> None: | ||
| super().normalize(archive, logger) | ||
| # self.name = self.m_def.name |
There was a problem hiding this comment.
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', |
| - 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. |
There was a problem hiding this comment.
move TODO outside of description
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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']: |
| density = Quantity( | ||
| type=MEnum('relaxed', 'unrelaxed'), | ||
| description=""" | ||
| unrelaxed density: MP2 expectation value density |
There was a problem hiding this comment.
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! |
There was a problem hiding this comment.
move TODO out of description
| """, | ||
| ) | ||
|
|
||
| integration_thresh = Quantity( |
There was a problem hiding this comment.
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. |
| core_threshold = Quantity( | ||
| type=np.float64, | ||
| description=""" | ||
| the energy window for the first occupied MO to be localized (in a.u.). |
ndaelman-hu
left a comment
There was a problem hiding this comment.
Covered model_method and mode_system now. Will do the rest in a future review.
You can resolve these comments in the meantime.
| symmetry_label = Quantity( | ||
| type=str, description='Symmetry label of the molecular orbital.' | ||
| ) |
There was a problem hiding this comment.
consider type=MEnum, so you can better distinguish with the atoms state
| A section representing a single molecular orbital. | ||
| """ | ||
|
|
||
| energy = Quantity( |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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'), |
There was a problem hiding this comment.
Note that this does not yet provide any logic for saving the user input.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
Pls elaborate a bit more here. Feel free to also add references for specific methods.
| def normalize(self, archive: 'EntryArchive', logger: 'BoundLogger') -> None: | ||
| super().normalize(archive, logger) |
There was a problem hiding this comment.
The normalization is redundant here
| | `'BW'` | Brillouin-Wigner | | ||
| """, | ||
| a_eln=ELNAnnotation(component='EnumEditQuantity'), | ||
| ) # TODO: check if the special symbols are supported |
There was a problem hiding this comment.
Latex is indeed supported
There was a problem hiding this comment.
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.
Pull Request Test Coverage Report for Build 13011770671Details
💛 - Coveralls |
…rtreeFock section
|
not entirely relevant, but I wish to keep it for a while |
The first part of ModelMethod contributions for chemistry.
Up until now:
HartreeFock
PerturbationMethod
MolecularOrbital
LCAO
LocalCorrelation
AtomCenteredBasisSet
GTOIntegralDecomposition
NumericalIntegration
OrbitalLocalization