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

Improve types for electronic_structure.{bandstructure/cohp} #3873

Merged
merged 63 commits into from
Aug 3, 2024

Conversation

DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented Jun 10, 2024

Summary

  • Improve type annotations and docstring for electronic_structure
    • bandstructure
    • cohp
  • Replace unnecessary single-item list extend with append in 8e25238 for performance

Caution

Change the cell translation vector translation from list[float] to tuple[float, float, float] (Vector3D) in 9551db2 and 5fcf236

New custom type

Script for testing lobsterpy

Bash script
#!/bin/bash -l

CONDA_PROFILE="/opt/anaconda3/etc/profile.d/conda.sh"
ENV_NAME="test_lobster"
PYTHON_VERSION="3.11"
PMG_REPO="https://github.com/DanielYang59/pymatgen.git"
PMG_BRANCH="type-elec-struct"
LOBSTERPY_REPO="https://github.com/JaGeo/LobsterPy.git"

# Functions
function create_and_activate_env() {
  conda create -y -n $ENV_NAME python=$PYTHON_VERSION
  conda activate $ENV_NAME
}

function clone_and_install_pymatgen() {
  git clone --depth 1 -b $PMG_BRANCH $PMG_REPO
  cd pymatgen
  pip install .
  cd ..
}

function clone_and_install_lobsterpy() {
  git clone --depth 1 $LOBSTERPY_REPO
  cd LobsterPy
  pip install -e '.[featurizer,dev,tests]'
  pip install pytest-xdist
  cd ..
}

function run_pytest() {
  cd LobsterPy
  pytest tests --maxfail=1000
  cd ..
}

function remove_env() {
  conda deactivate
  conda env remove -n $ENV_NAME -y
}

# Main Script
source $CONDA_PROFILE
create_and_activate_env
clone_and_install_pymatgen
clone_and_install_lobsterpy
run_pytest
remove_env

@@ -1057,7 +1079,26 @@ def get_projections_on_elements_and_orbitals(self, el_orb_spec):
return result


def get_reconstructed_band_structure(list_bs, efermi=None):
@overload
def get_reconstructed_band_structure( # type: ignore[overload-overlap]
Copy link
Contributor Author

@DanielYang59 DanielYang59 Jun 11, 2024

Choose a reason for hiding this comment

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

Need a closer look at this type error, perhaps it's because mypy might be false-positive in determining overlapping types python/mypy#4020.

@DanielYang59 DanielYang59 changed the title Improve type annotations for electronic_structure Improve type annotations for electronic_structure.{bandstructure/cohp} Jun 12, 2024
@DanielYang59 DanielYang59 changed the title Improve type annotations for electronic_structure.{bandstructure/cohp} Improve types for electronic_structure.{bandstructure/cohp} Jun 12, 2024
atom1 (str): The first atom that contributes to the bond.
atom2 (str): The second atom that contributes to the bond.
length (float): Bond length.
translation: translation list, e.g. [0, 0, 0].
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit unclear to me what is translation? Is it a 3D vector?

Copy link
Member

Choose a reason for hiding this comment

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

Same wording is used as in Lobster. It describes the unit cell in which the atom is in. [0 0 0] is the normal cell, but the atom might be translated to the neighboring cell

Copy link
Contributor Author

@DanielYang59 DanielYang59 Jun 12, 2024

Choose a reason for hiding this comment

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

Thanks for clarifying! I just searched the wrong documentation ;) I searched Lobster_FAQ_5.0.0 instead of Lobster_Users_Guide_5.0.0.

If atoms are not located in the original unit cell, the corresponding translation vector can be added by the keyword "cell" like this:
cobiBetween atom 1 atom 2 atom 3 cell 1 0 0

Update: clarified in 0e75e0d

@DanielYang59 DanielYang59 force-pushed the type-elec-struct branch 3 times, most recently from cb427d7 to 94ea368 Compare July 1, 2024 15:05
Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

thanks a lot @DanielYang59 for your continued clean up efforts! 👍

@janosh janosh merged commit 0ebaa64 into materialsproject:master Aug 3, 2024
33 checks passed
@DanielYang59 DanielYang59 deleted the type-elec-struct branch August 3, 2024 13:55
@DanielYang59
Copy link
Contributor Author

All good, thanks for reviewing. I would clean up the merge conflicts in #3880 to get it ready for review. Thanks for your time :)

@janosh janosh added the types Type all the things label Aug 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
types Type all the things
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants