Add types for `core.(molecular_orbitals|operations|sites|spectrum|tensor|xcfunc)` by DanielYang59 · Pull Request #3829 · materialsproject/pymatgen

DanielYang59 · 2024-05-14T03:27:52Z

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.

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

pymatgen/core/operations.py

DanielYang59 · 2024-05-14T09:20:05Z

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

pymatgen/pymatgen/core/operations.py

Lines 92 to 93 in 51180fc

    
           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.

This reverts commit bf2a953.

janosh

thank a lot @DanielYang59! 👍

DanielYang59 · 2024-05-14T13:22:30Z

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

janosh · 2024-05-14T13:29:59Z

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 · 2024-05-14T13:36:49Z

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 :)

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>

* 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>

…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]`

DanielYang59 added 2 commits May 14, 2024 11:14

fix core.molecular_orbitals

9e81055

fix core.operations

d26f117

DanielYang59 commented May 14, 2024

View reviewed changes

pymatgen/core/operations.py Outdated Show resolved Hide resolved

DanielYang59 added 8 commits May 14, 2024 11:43

add types for site, mypy errors to fix

6c7e0ec

remove no_type_check decorator

5fea236

move dunder methods to the top

6624aeb

fix mypy errors

cbc4b69

remove debug tag

05df6fa

improve spectrum

03cdacf

finish core.tensors

6779c6d

finish xcfunc

41e5851

DanielYang59 changed the title ~~Add types for core~~ Add types for core.molecular_orbitals/operations/sites/spectrum/tensor/xcfunc May 14, 2024

DanielYang59 added 2 commits May 14, 2024 16:57

fix hash of SymmOp

bf2a953

add a missing type

6e98ca2

DanielYang59 added 9 commits May 14, 2024 19:09

Revert "fix hash of SymmOp"

a5b08a3

This reverts commit bf2a953.

fix some types in operations

18e1d48

"TEST": remove one unused var, relocate another

a5ec9f7

final tweak types

6cffe88

avoid hard-code class name

19ffa1d

format tweaks

e94766e

type tweak

ef6c7ed

fix unit test

7397b6f

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

8b8532d

DanielYang59 marked this pull request as ready for review May 14, 2024 12:37

DanielYang59 requested review from janosh, mkhorton and shyuep as code owners May 14, 2024 12:37

janosh approved these changes May 14, 2024

View reviewed changes

janosh added the housekeeping Moving around or cleaning up old code/files label May 14, 2024

janosh added the types Type all the things label May 14, 2024

janosh merged commit 616abc5 into materialsproject:master May 14, 2024

janosh mentioned this pull request May 14, 2024

Guard TYPE_CHECKING only imports #3827

Merged

DanielYang59 deleted the add-type-for-core-remaining branch May 14, 2024 13:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

Add types for `core.(molecular_orbitals|operations|sites|spectrum|tensor|xcfunc)`#3829
janosh merged 21 commits intomaterialsproject:masterfrom
DanielYang59:add-type-for-core-remaining

DanielYang59 commented May 14, 2024 •

edited

Loading

Uh oh!

Uh oh!

DanielYang59 commented May 14, 2024

Uh oh!

janosh left a comment

Uh oh!

DanielYang59 commented May 14, 2024

Uh oh!

janosh commented May 14, 2024 •

edited

Loading

Uh oh!

DanielYang59 commented May 14, 2024 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

DanielYang59 commented May 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

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

PR too large, following separated to another PR

Uh oh!

Uh oh!

DanielYang59 commented May 14, 2024

Uh oh!

janosh left a comment

Choose a reason for hiding this comment

Uh oh!

DanielYang59 commented May 14, 2024

Uh oh!

janosh commented May 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanielYang59 commented May 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

DanielYang59 commented May 14, 2024 •

edited

Loading

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

janosh commented May 14, 2024 •

edited

Loading

DanielYang59 commented May 14, 2024 •

edited

Loading