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

Adding ModelMethod for TB Wannier90 #8

Merged
merged 38 commits into from
Mar 4, 2024

Conversation

JosePizarro3
Copy link
Collaborator

@JosePizarro3 JosePizarro3 commented Feb 20, 2024

@ndaelman-hu @JFRudzinski

A brief intro before our meeting tomorrow 27.02.2024.

This is my initial version on defining ModelMethod, some base classes which will be contained within, and some usage in the specific case of TB models.

As you can see:

  • ModelMethod serves as the base class from which to inherit to specific methodologies (e.g., DFT should also be inheriting from it, as well as GW, BSE, ForceFields(?), etc.).
  • It also contains info about meshes, self-consistency (this is the former schema Scf class), and proxy references to other ModelMethod sections already existing in the archive.

I deleted some quantities and sections which were not so clear or unused:

  • Smearing
  • Electronic
  • MoleculeParameters
  • LatticeModelHamiltonian
  • Method.stress_tensor_method, and I defined methods_ref as a generic way of coverng all method references under a given ModelMethod.
  • Scf.native_tier

I am still working out the details of TB, but there you can see the usage:

  • Of referencing specific sections in ModelSystem
  • Of using this class for further inheritance (there are different types of TB models, Wannier, SlaterKoster, xTB).
  • If you just check the old schema, you would see how complex SlaterKoster was to define, as Mohammad (who was the one doing this) was developing this it was very complex due to the problem of composing TB under ModelMethod. Now, I'll argue that it works much more nicely and it is easily usable by external people.

Now, I will work a bit further on defining the core methodologies (DFT, GW, BSE, DMFT), and you might wanna take over to define your task-specific problems.

@JosePizarro3 JosePizarro3 self-assigned this Feb 20, 2024
@JosePizarro3 JosePizarro3 linked an issue Feb 20, 2024 that may be closed by this pull request
@JosePizarro3 JosePizarro3 force-pushed the 3-adding-modelmethod-for-tb-wannier90-2 branch 2 times, most recently from 49a6118 to 04bb96b Compare February 26, 2024 09:16
@JosePizarro3
Copy link
Collaborator Author

@ndaelman-hu @JFRudzinski I updated the comment in the description and added you as reviewers. Let's chat tomorrow.

Improved ModelSystem description
Moved lattice_vectors_reciprocal to KMesh

Added normalizations
Deleted unused comment in ModelSystem

Clean up descriptions in model_system.py
Added self.name under TB
Added common.py for common sections
Improved code writing for assignements in atoms_state.py
… active_atoms in ModelSystem for TB class

Improved SlaterKoster method and added normalization for bond name extraction
@JosePizarro3 JosePizarro3 force-pushed the 3-adding-modelmethod-for-tb-wannier90-2 branch from 04bb96b to b398c96 Compare February 26, 2024 09:32
@JosePizarro3
Copy link
Collaborator Author

@ndaelman-hu @JFRudzinski would you mind reviewing this PR?

You can ignore the changes in atoms_state.py. This is done because of the unit testing showed some errors in that schema that I needed to fix.

Everything else in model_method.py is new. Like we talked, I left out everything related with BasisSet and ForceField. I am also skipping the implementation of some functions in DMFT, because first I need to see how the schema behaves as a whole.

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 is great! Just to bring my main questions to the forefront:

  1. When should we be using Enum values for method names?
  2. When should we be adding eln annotations to quantities?
  3. What is our overall strategy for referencing between methods and the other main sections (workflows, systems, outputs)?

simulationdataschema/atoms_state.py Show resolved Hide resolved
simulationdataschema/model_method.py Show resolved Hide resolved
simulationdataschema/model_method.py Outdated Show resolved Hide resolved
simulationdataschema/model_method.py Outdated Show resolved Hide resolved
simulationdataschema/model_method.py Outdated Show resolved Hide resolved
simulationdataschema/model_method.py Outdated Show resolved Hide resolved
simulationdataschema/model_method.py Outdated Show resolved Hide resolved
@ndaelman-hu
Copy link
Collaborator

@JosePizarro3 I'll do the review this evening, okay?

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.

Hey! Very nice work. Mostly small stuff.

Tbh, I didn't check the methods after TB much, but I can see to help there with the testing of the normalizers.
I will brainstorm a bit on how to improve the DFT section too.

.github/workflows/actions.yaml Outdated Show resolved Hide resolved
simulationdataschema/atoms_state.py Show resolved Hide resolved
simulationdataschema/atoms_state.py Outdated Show resolved Hide resolved
simulationdataschema/atoms_state.py Outdated Show resolved Hide resolved
simulationdataschema/atoms_state.py Outdated Show resolved Hide resolved
simulationdataschema/model_method.py Outdated Show resolved Hide resolved
simulationdataschema/model_method.py Outdated Show resolved Hide resolved
simulationdataschema/model_method.py Outdated Show resolved Hide resolved
simulationdataschema/model_method.py Show resolved Hide resolved
simulationdataschema/model_method.py Outdated Show resolved Hide resolved
Added github action for Ruff check and formatting
@JosePizarro3 JosePizarro3 force-pushed the 3-adding-modelmethod-for-tb-wannier90-2 branch from 08fa77c to 47a471c Compare March 1, 2024 13:41
@JosePizarro3 JosePizarro3 force-pushed the 3-adding-modelmethod-for-tb-wannier90-2 branch from d8054ce to df210d5 Compare March 1, 2024 14:05
@JosePizarro3
Copy link
Collaborator Author

Thanks a lot for your reviews!! 👏🏻

@ndaelman-hu @JFRudzinski would you mind to quickly checking again? Focus on your comments, close them if you feel satisfied, and if everything is good, I will merge.

@ndaelman-hu
Copy link
Collaborator

Thanks a lot for your reviews!! 👏🏻

@ndaelman-hu @JFRudzinski would you mind to quickly checking again? Focus on your comments, close them if you feel satisfied, and if everything is good, I will merge.

I was just about to head home. Do you mind if I do it tomorrow morning?

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.

Okay, all done! I closed all that is resolved from my side. Just check the open comments.

I'm really eager to get hands-on with this schema, as now we will have much clearer business logic.

simulationdataschema/model_method.py Outdated Show resolved Hide resolved
simulationdataschema/model_method.py Outdated Show resolved Hide resolved
Added utils is_not_representative

Redefine QuasiparticlesFrequencyMesh

Changed resolve_points to resolve_points_and_offset in KMesh
@JosePizarro3
Copy link
Collaborator Author

Thanks a lot for the reviews @JFRudzinski @ndaelman-hu !! 🥳

I merge this and work on other minor topics of the repo.

@JosePizarro3 JosePizarro3 merged commit 9193c24 into develop Mar 4, 2024
3 checks passed
@JosePizarro3 JosePizarro3 deleted the 3-adding-modelmethod-for-tb-wannier90-2 branch March 4, 2024 12:08
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.

Adding ModelMethod for TB Wannier90
3 participants