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

Add types for core.(molecular_orbitals|operations|sites|spectrum|tensor|xcfunc) #3829

Merged

Conversation

DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented May 14, 2024

Summary

  • Add/improve types for core
  • Replace hard-coded class call with type(self)
  • Replace all [0:x] with [:x]

Add types for remaining part of core, follow up of #3814.

  • core.molecular_orbitals
  • core.operations
  • core.sites
  • core.spectrum
  • core.tensor
  • core.xcfunc

PR too large, following separated to another PR

  • core.structure
  • core.units
  • core.trajectory
  • Use type(self) to avoid hard-coded class name in __eq__ and similar methods

@DanielYang59 DanielYang59 changed the title Add types for core Add types for core.molecular_orbitals/operations/sites/spectrum/tensor/xcfunc May 14, 2024
@DanielYang59
Copy link
Contributor Author

For some reason, replacing the magic __hash__ method of SymmOp in bf2a953 breaks a unit test of core.structure (test_from_prototype):

def __hash__(self) -> int:
return 7

There must be something in it that depends on the hash that I'm not aware of, and I totally have no idea why.

@DanielYang59 DanielYang59 marked this pull request as ready for review May 14, 2024 12:37
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.

thank a lot @DanielYang59! 👍

@janosh janosh added the housekeeping Moving around or cleaning up old code/files label May 14, 2024
@janosh janosh added the types Type all the things label May 14, 2024
@janosh janosh changed the title Add types for core.molecular_orbitals/operations/sites/spectrum/tensor/xcfunc Add types for core.(molecular_orbitals|operations|sites|spectrum|tensor|xcfunc) May 14, 2024
@janosh janosh merged commit 616abc5 into materialsproject:master May 14, 2024
22 of 23 checks passed
@DanielYang59
Copy link
Contributor Author

Thanks for reviewing. Do you have any comments on #3829 (comment)?

@DanielYang59 DanielYang59 deleted the add-type-for-core-remaining branch May 14, 2024 13:28
@janosh
Copy link
Member

janosh commented May 14, 2024

not sure how the SymmOP.__hash__ comes into play in that test. you could modify this assert to print actual and expected string to see how they differ

- assert str(struct) == expected_struct_str
+ assert str(struct) == expected_struct_str, f"{struct=}, {expected_struct_str=}"

@DanielYang59
Copy link
Contributor Author

DanielYang59 commented May 14, 2024

Thanks for the suggestion, this is what I got:

> print(struct)
Full Formula (Li4 Cl4)
Reduced Formula: LiCl
abc   :   2.560000   2.560000   2.560000
angles:  90.000000  90.000000  90.000000
pbc   :       True       True       True
Sites (8)
  #  SP      a    b    c
---  ----  ---  ---  ---
  0  Li    0    0    0
  1  Li    0    0.5  0.5
  2  Li    0.5  0    0.5
  3  Li    0.5  0.5  0
  4  Cl    0    0.5  0.5
  5  Cl    0.5  0    0.5
  6  Cl    0    0    0
  7  Cl    0.5  0.5  0

> print(expected_struct_str)
Full Formula (Li4 Cl4)
Reduced Formula: LiCl
abc   :   2.560000   2.560000   2.560000
angles:  90.000000  90.000000  90.000000
pbc   :       True       True       True
Sites (8)
  #  SP      a    b    c
---  ----  ---  ---  ---
  0  Li    0    0    0
  1  Li    0.5  0.5  0
  2  Li    0.5  0    0.5
  3  Li    0    0.5  0.5
  4  Cl    0.5  0    0.5
  5  Cl    0    0.5  0.5
  6  Cl    0.5  0.5  0
  7  Cl    0    0    0

Looks like the ordering of sites changed, and I don't quite know how hash of SymmOp is related to generation of Structure. I would look closer later :)

DanielYang59 added a commit to DanielYang59/pymatgen that referenced this pull request May 16, 2024
fix `core.operations`

add types for site, mypy errors to fix

remove no_type_check decorator

move dunder methods to the top

fix mypy errors

remove debug tag

improve `spectrum`

finish `core.tensors`

finish `xcfunc`

fix hash of `SymmOp`

finish `core.trajectory`

Add types for `core.molecular_orbitals/operations/sites/spectrum/tensor/xcfunc`  (materialsproject#3829)

* fix `core.molecular_orbitals`

* fix `core.operations`

* add types for site, mypy errors to fix

* remove no_type_check decorator

* move dunder methods to the top

* fix mypy errors

* remove debug tag

* improve `spectrum`

* finish `core.tensors`

* finish `xcfunc`

* fix hash of `SymmOp`

* add a missing type

* Revert "fix hash of `SymmOp`"

This reverts commit bf2a953.

* fix some types in operations

* "TEST": remove one unused var, relocate another

* final tweak types

* avoid hard-code class name

* format tweaks

* type tweak

* fix unit test

* replace all `[0:x]` with `[:x]`

Guard `TYPE_CHECKING` only imports (materialsproject#3827)

* flake8 --select=TYP001

* fix type import

* format docstring to google style

* fix more unguarded typecheck imports

* format more docs to Google format

* trigger CI (better var names)

---------

Co-authored-by: Janosh Riebesell <janosh.riebesell@gmail.com>

move structures out of util dir (materialsproject#3831)

Add matgenb in the aux link section. Because apparently it is not
"discoverable". Fixes materialsproject#3811.

More updates.

Move AIMS notebooks to matgenb.

Add README in examples folder that point to proper resources.

Improve type annotations and comments for `io.cif` (materialsproject#3820)

* remove duplicate warning

* type and docstring tweaks

* add some types for io.cif

* add comments and types

* temp save of some formatting

* revert functional change

* revert unrelated changes

* fix unit test

* remove update that does nothing

* relocate `sub_space_group` and `space_groups` to their usage

* add more types

* pre-commit auto-fixes

* breaking: make parse_magmom/oxi_stats private

* remove merge header

* fix unit test

* add more types

* fix mypy errors

* add a few spaces

* remove if name ==main

* simplify "check to see if" in comments

* final tweaks

* revise docstring

* replace deprecated abc.abstractproperty

* add missing doc strings and standardize existing

* breaking: fix typo in method name orthongonalize_structure

* revert accidental change in test_composition.py

---------

Co-authored-by: Janosh Riebesell <janosh.riebesell@gmail.com>
janosh added a commit that referenced this pull request May 17, 2024
* relocate dunder methods to top for core.trajectory

fix `core.operations`

add types for site, mypy errors to fix

remove no_type_check decorator

move dunder methods to the top

fix mypy errors

remove debug tag

improve `spectrum`

finish `core.tensors`

finish `xcfunc`

fix hash of `SymmOp`

finish `core.trajectory`

Add types for `core.molecular_orbitals/operations/sites/spectrum/tensor/xcfunc`  (#3829)

* fix `core.molecular_orbitals`

* fix `core.operations`

* add types for site, mypy errors to fix

* remove no_type_check decorator

* move dunder methods to the top

* fix mypy errors

* remove debug tag

* improve `spectrum`

* finish `core.tensors`

* finish `xcfunc`

* fix hash of `SymmOp`

* add a missing type

* Revert "fix hash of `SymmOp`"

This reverts commit bf2a953.

* fix some types in operations

* "TEST": remove one unused var, relocate another

* final tweak types

* avoid hard-code class name

* format tweaks

* type tweak

* fix unit test

* replace all `[0:x]` with `[:x]`

Guard `TYPE_CHECKING` only imports (#3827)

* flake8 --select=TYP001

* fix type import

* format docstring to google style

* fix more unguarded typecheck imports

* format more docs to Google format

* trigger CI (better var names)

---------

Co-authored-by: Janosh Riebesell <janosh.riebesell@gmail.com>

move structures out of util dir (#3831)

Add matgenb in the aux link section. Because apparently it is not
"discoverable". Fixes #3811.

More updates.

Move AIMS notebooks to matgenb.

Add README in examples folder that point to proper resources.

Improve type annotations and comments for `io.cif` (#3820)

* remove duplicate warning

* type and docstring tweaks

* add some types for io.cif

* add comments and types

* temp save of some formatting

* revert functional change

* revert unrelated changes

* fix unit test

* remove update that does nothing

* relocate `sub_space_group` and `space_groups` to their usage

* add more types

* pre-commit auto-fixes

* breaking: make parse_magmom/oxi_stats private

* remove merge header

* fix unit test

* add more types

* fix mypy errors

* add a few spaces

* remove if name ==main

* simplify "check to see if" in comments

* final tweaks

* revise docstring

* replace deprecated abc.abstractproperty

* add missing doc strings and standardize existing

* breaking: fix typo in method name orthongonalize_structure

* revert accidental change in test_composition.py

---------

Co-authored-by: Janosh Riebesell <janosh.riebesell@gmail.com>

* remove accidental change

* fix mypy errors

* fix mypy error

* add some for core.units

* rename `ArrayWithFloatWithUnit` to `ArrayWithUnit` in comment

* fix mypy errors

* tweak docstring for units

* nest internal funcs for units

* simplify module docstring

* revert to accessing private attrib

* fix unit test and revert support for `Unit`

* use `str` over `Unit`

* support unit as both str and Unit

* tweak docstring example

* improve IndexError messages

* test_index_error

---------

Co-authored-by: Janosh Riebesell <janosh.riebesell@gmail.com>
janosh pushed a commit to esoteric-ephemera/pymatgen that referenced this pull request May 17, 2024
…or/xcfunc` (materialsproject#3829)

* fix `core.molecular_orbitals`

* fix `core.operations`

* add types for site, mypy errors to fix

* remove no_type_check decorator

* move dunder methods to the top

* fix mypy errors

* remove debug tag

* improve `spectrum`

* finish `core.tensors`

* finish `xcfunc`

* fix hash of `SymmOp`

* add a missing type

* Revert "fix hash of `SymmOp`"

This reverts commit bf2a953.

* fix some types in operations

* "TEST": remove one unused var, relocate another

* final tweak types

* avoid hard-code class name

* format tweaks

* type tweak

* fix unit test

* replace all `[0:x]` with `[:x]`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
housekeeping Moving around or cleaning up old code/files types Type all the things
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants