Skip to content

Conversation

@EBB2675
Copy link
Collaborator

@EBB2675 EBB2675 commented May 12, 2025

No description provided.

@coveralls
Copy link

coveralls commented May 12, 2025

Pull Request Test Coverage Report for Build 16907926789

Details

  • 189 of 231 (81.82%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.8%) to 70.496%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/nomad_simulations/schema_packages/numerical_settings.py 16 17 94.12%
src/nomad_simulations/schema_packages/properties/molecular_orbitals.py 29 31 93.55%
src/nomad_simulations/schema_packages/basis_set.py 52 59 88.14%
src/nomad_simulations/schema_packages/model_method.py 62 94 65.96%
Totals Coverage Status
Change from base Build 16906847440: 0.8%
Covered Lines: 2148
Relevant Lines: 3047

💛 - Coveralls

@EBB2675 EBB2675 marked this pull request as ready for review May 12, 2025 13:09
@JFRudzinski
Copy link
Collaborator

will review tomorrow 😅

@EBB2675
Copy link
Collaborator Author

EBB2675 commented May 13, 2025

hi again,

this is the basis for Quantum Chemistry support. Later on, I can build all the remaining methods while using these building blocks.

When you are done with reviewing the QC-base, I will then add all the remaining (single-reference) post-HF methods.

""",
)

function_type = Quantity(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is "type" clear enough, or should ang-mom someone be in the name?

""",
)

exponents = Quantity(
Copy link
Collaborator

Choose a reason for hiding this comment

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

"primitive_exponents"? I just have no idea of the context/clarity, so feel free to reject these naming questions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

both TREXIO and basis set exchange library uses exponents

)

primitive_normalization = Quantity(
type=MEnum('L2', 'none', 'contracted', 'custom'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move none to the end of the list

Copy link
Collaborator

Choose a reason for hiding this comment

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

You might want to use all lower case for consistency ie l2


primitive_normalization = Quantity(
type=MEnum('L2', 'none', 'contracted', 'custom'),
default='L2',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would have thought the default should be none?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

its kind of a standard convention that each primitive basis function is L²-normalized before forming any contracted orbital

Copy link
Collaborator

Choose a reason for hiding this comment

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

Even so, I think it is not good practice to populated with a parameter that is not explicitly parsed from the files. It's safer to leave the default out so that it is not populated unless this metadata is actually there

description="""
**Extra scaling factors for each primitive** (dimensionless).
- Let ϕₖ(r) be the *analytic* primitive basis function (Gaussian or
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you check at some point if the greek letters are displayed properly in the GUI metainfo browser and DATA tab?

contracted function so that
\\[
\\int_{\\mathbb{R}^3} |χ(r)|^2 \\,dr = 1.
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here, does this display? I remember looking into putting equations in the descriptions but I can't remember the outcome now.

I'm not suggesting you change anything, but if it doesn't display in the GUI we could talk to Area D about a feature for this

"""
super().normalize(archive, logger)

# ---------------- infer component / primitive counts -------------
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this syntax for section separation comments, very nice readability 👍

from nomad_simulations.schema_packages.variables import Variables


class MolecularOrbitals(PhysicalProperty):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a "main" value for this property? Does it even make sense in this case?

I am wondering in the context of using value or some sub-section for different types of the "main" value. I guess we never discussed what to do if the property is only really defined in terms of a set of equally-important metadata

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.

This has developed into a really nice, clear and extensive(-looking) schema 🙌 The comments and descriptions are particularly good, these should serve as a reference point as we continue to refine and extend the schema

I presume that at this point Denis is already well versed with the schema and approves? I think you said you would also run it by your PhD advisor? Or will you do that later? And what about Nathan, did he review it?

Let's finalize all these things and also a few of my below comments later today, and then I would say you can go ahead and merge it


# ---------------- property: matrix view (read‑only) ------------------
@property
def coeff_matrix(self) -> np.ndarray:
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 really not a section quantity? at least the name looks like.

""",
)

functional_composition = SubSection(
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 plural? what is the convention when sub-sections repeat?

@EBB2675 EBB2675 force-pushed the qc-single-reference branch from 0e2a43e to 609f5c7 Compare August 12, 2025 11:48
This was unlinked from issues Nov 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

6 participants