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

Handling of labels in AtomicCell #7

Merged
merged 15 commits into from
Feb 20, 2024

Conversation

JosePizarro3
Copy link
Collaborator

@JosePizarro3 JosePizarro3 commented Feb 19, 2024

@JFRudzinski @ndaelman-hu please, review when you can this merge request.

I worked out a bit further on the labels, atomic_numbers and Method.AtomParameters ideas and came with the following schema. Just a few points to consider:

  1. Now there is further abstraction by having a class Cell. This can be used to inherit other system cells, e.g., CoarseGrainedCell or LatticeModelCell, without the need of pasing real chemical elements information and normalization.
  2. The concepts of labels and atomic_numbers have been moved under a new base class named AtomsState. This base class is a combination of these concepts together with the old schema for AtomParameters. Everything in this context appears then under ModelSystem from now on.
  3. This new base class also contains: information about the orbital state, information about the atom charge (as a positive or negative integer, tho I am happy to hear feedback here in case this might be a float), information about CoreHole and HubbardInteractions.
  4. The design idea is that OrbitalsState has the information of the relevant orbitals for each atom. This means that not all orbitals for an atom (i.e., 1s, 2px, 2py, etc.) need to be necessarily parsed, but rather the ones relevant for the use-case being treated. The example of use-cases could be CoreHole and HubbardInteractions.
  5. For HubbardInteractions, now there is a list of orbitals_ref which then points to the specific orbitals used for the interactions. This means that HubbardInteractions.umn shape is defined by the length of these refs.
  6. For CoreHole, I also added orbital_ref. It is now then a game of adding n_excited_electrons and checking the orbital_ref.degeneracy to resolve the occupation. Prior to this, CoreHole inherited from the state, but I think this is much more clean, as now CoreHole only contains info relevant for when creating a hole in a core orbital state.
  7. I added RussellSaundersState under utils.py. I am still trying to debug and wrap my head around what are these methods using, but it shouldn't be too complicated. I would be ok too to move it to its own module, like you prefer.
  8. I will deprecate the old method MoleculeParameters, but we can think if there are some use-cases how to implement it. It shouldn't be too complicated.
  9. I am considering the old method Pseudopotentials to probably live under a ModelMethod.BasisSet class. But we (@ndaelman-hu and I) can discuss this a bit later. I am happy to hear more feedback tho.

I hope you like this. With this, I think ModelSystem is fully flexible, normalizations are properly ran, and I will continue working on the #3 ModelMethod schema.

Edit:
The corresponding branch in Gitlab, electronic-parsers, and some La2CuO4_test_basesections_Wannier90.zip.

@JosePizarro3 JosePizarro3 added the new feature New feature or request label Feb 19, 2024
@JosePizarro3 JosePizarro3 self-assigned this Feb 19, 2024
@JosePizarro3 JosePizarro3 linked an issue Feb 19, 2024 that may be closed by this pull request
@JosePizarro3
Copy link
Collaborator Author

Btw, @ndaelman-hu, from where did you defined the orbital map? The dictionary that defines l: ml. I am curious to check the reference.

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 looks great @JosePizarro3 , it appears to me to be more or less what we discussed in your office, and I think it makes a lot of sense to me.

Just a small comment about the use of time_step.

simulationdataschema/model_system.py Outdated Show resolved Hide resolved
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.

All minor stuff. Just take a look, no need for additional review.

One question: what if we have basic building blocks larger than atoms, i.e. molecules or coarse-grained? They'll need their own State, similar to AtomState, I reckon...
Maybe add a generic base class too.

Comment on lines 345 to 353
umn = Quantity(
type=np.float64,
shape=["*", "*"],
unit="joule",
description="""
Value of the local Hubbard interaction matrix. The order of the rows and columns coincide
with the elements in `orbital_ref`.
""",
)
Copy link
Collaborator

@ndaelman-hu ndaelman-hu Feb 19, 2024

Choose a reason for hiding this comment

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

I know that this is just a code migration, but I don't remember reviewing these extensions to the Hubbard parameters.

My only remark is about the naming. I really had no idea what umn was supposed to represent, until I read the description. Maybe you can make it clearer?
Moreover, are there any limitations on the matrix dimensions? Maybe the no. rows = l-degeneracy (at least in most DFT codes)?

Copy link
Collaborator Author

@JosePizarro3 JosePizarro3 Feb 19, 2024

Choose a reason for hiding this comment

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

I'll rename it u_matrix. I will also rename other quantities like u_interaction, j_hunds_coupling, u_interorbital_interaction, j_local_exchange_interaction. I think u_effective can remain as it is.

Yes, the shape of this matrix coincides with the length of orbitals_ref.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, the shape of this matrix coincides with the length of orbitals_ref.

Maybe we should consider adding the shapes again? Let's at least keep it in the description for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I add them back. The checks are not being performed, but this is something we should discuss further.

simulationdataschema/atoms_state.py Outdated Show resolved Hide resolved
simulationdataschema/atoms_state.py Outdated Show resolved Hide resolved
Comment on lines 426 to 480
def resolve_u_interactions(self, logger: BoundLogger) -> Optional[tuple]:
"""
Resolves the Hubbard interactions (u, up, jh) from the Slater integrals (F0, F2, F4).

Args:
logger (BoundLogger): The logger to log messages.

Returns:
(Optional[tuple]): The Hubbard interactions (u, up, jh).
"""
if self.slater_integrals is None or len(self.slater_integrals) == 3:
logger.warning(
"Could not find `slater_integrals` or the length is not three."
)
return None
f0 = self.slater_integrals[0]
f2 = self.slater_integrals[1]
f4 = self.slater_integrals[2]
u_interaction = (
((2.0 / 7.0) ** 2)
* (f0 + 5.0 * f2 + 9.0 * f4)
/ (4.0 * np.pi)
* ureg("joule")
)
up_interaction = (
((2.0 / 7.0) ** 2)
* (f0 - 5.0 * f2 + 3.0 * f4 / 2.0)
/ (4.0 * np.pi)
* ureg("joule")
)
jh_interaction = (
((2.0 / 7.0) ** 2)
* (5.0 * f2 + 15.0 * f4 / 4.0)
/ (4.0 * np.pi)
* ureg("joule")
)
return u_interaction, up_interaction, jh_interaction

def resolve_u_effective(self, logger: BoundLogger) -> Optional[np.float64]:
"""
Resolves the effective U parameter (u - j).

Args:
logger (BoundLogger): The logger to log messages.

Returns:
(Optional[np.float64]): The effective U parameter.
"""
if self.u is None or self.j is None:
logger.warning(
"Could not find `HubbardInteractions.u` or `HubbardInteractions.j`."
)
return None
return self.u - self.j

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice! Do we have unit tests for these?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not yet, maybe we can tackle testing soon in another branch. For now I am testing directly on a parser and checking that everything goes well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mind opening this as an issue? Maybe you can also share your ideas of what you are expecting for testing and we discuss there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can also take it after you merge this and add you as a reviewer?
I'll open the issue this evening. Just remind me if I forget.

simulationdataschema/atoms_state.py Outdated Show resolved Hide resolved
simulationdataschema/model_system.py Outdated Show resolved Hide resolved
simulationdataschema/model_system.py Outdated Show resolved Hide resolved
simulationdataschema/model_system.py Outdated Show resolved Hide resolved
simulationdataschema/model_system.py Show resolved Hide resolved
simulationdataschema/utils/utils.py Outdated Show resolved Hide resolved
@JosePizarro3 JosePizarro3 merged commit a22e918 into develop Feb 20, 2024
2 checks passed
@JosePizarro3 JosePizarro3 deleted the 5-handling-of-labels-in-atomiccell branch February 20, 2024 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handling of labels in AtomicCell
3 participants