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.

Sign up for GitHub

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

Jump to bottom

Guard TYPE_CHECKING only imports #3827

Merged
merged 9 commits into from
May 14, 2024
Merged

Guard TYPE_CHECKING only imports #3827

merged 9 commits into from
May 14, 2024

Conversation

DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented May 13, 2024 •
edited
Loading

Summary

  • Guard TYPE_CHECKING only imports
  • Format docstring to Google style in io.aims.sets

Can we automate this check (and fix)?

flake8 provides TYP001 rule to check this, but doesn't seem able to correct it automatically. Have to run flake8 --select=TYP001 and fix manually.

ruff might have this implemented but I didn't find it in the available rules.

@DanielYang59 DanielYang59 marked this pull request as ready for review May 14, 2024 03:46
@DanielYang59
Copy link
Contributor Author

@janosh. Please review this, thanks.

Wondering if we could automate this checking?

@janosh
Copy link
Member

janosh commented May 14, 2024

Wondering if we could automate this checking?

ruff has an open issue about implementing this rule which you could upvote and/or leave a comment

astral-sh/ruff#2302

@janosh
Copy link
Member

janosh commented May 14, 2024

this is conflicting now that #3829 is merged

@DanielYang59

This comment was marked as outdated.

Sign in to view
@DanielYang59
Copy link
Contributor Author

Resolved.

@DanielYang59 DanielYang59 marked this pull request as draft May 14, 2024 13:29
@DanielYang59 DanielYang59 marked this pull request as ready for review May 14, 2024 13:30
@janosh janosh added performance Some functionality is too slow or regressed types Type all the things labels May 14, 2024
@janosh janosh removed the performance Some functionality is too slow or regressed label May 14, 2024
@janosh
Copy link
Member

janosh commented May 14, 2024 •
edited
Loading

is the main benefit here meant to be code clarity (by clearly separating type-checking-time only imports)? i don't think we get any speedup since we're importing the typing module anyway to use TYPE_CHECKING

@DanielYang59
Copy link
Contributor Author

DanielYang59 commented May 14, 2024 •
edited
Loading

is the main benefit here meant to be code clarity? that type-checking-time only imports are clearly separated? i don't think we get any speedup since we're importing the typing module anyway to get TYPE_CHECKING

I would expect both if we could really complete this task (perhaps after a ruff rule is implemented), as I have seen quite some cases where a module is imported just for typing purpose (but it's too time consuming to check every import manually without a proper tool).

Currently I'm not sure if importing fewer would bring much benefit though, to be frank, sorry.

I opened this PR to try if I could identify those manually by running flake8 --select=TYP001 locally, but it seems it only detects a part of those unguarded imports, and looks like TYP001 might not be the right rule for this. 😞

This rule detects whether you've imported something from typing that's incompatible with your supported Python version.

@janosh janosh merged commit 40b0165 into materialsproject:master May 14, 2024
23 checks passed
@DanielYang59 DanielYang59 deleted the guard-typecheck-import branch May 14, 2024 14:54
@janosh janosh added the linting Linting and quality assurance label May 14, 2024
@DanielYang59
Copy link
Contributor Author

DanielYang59 commented May 14, 2024 •
edited
Loading

Thanks for reviewing. And sorry I'm unable to help completely resolve this (guard all imports by TYPE_CHECKING)

DanielYang59 added a commit to DanielYang59/pymatgen that referenced this pull request May 16, 2024
@DanielYang59 @janosh
relocate dunder methods to top for core.trajectory
1683c18 
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
@DanielYang59 @janosh
Improve type annotations for core.(trajectory/units) (#3832)
6a84d8a 
* 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 added a commit to esoteric-ephemera/pymatgen that referenced this pull request May 17, 2024
@DanielYang59 @janosh
Guard TYPE_CHECKING only imports (materialsproject#3827)
47df809 
* 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>
@DanielYang59 DanielYang59 mentioned this pull request Jun 1, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JaGeo JaGeo Awaiting requested review from JaGeo JaGeo is a code owner

@shyuep shyuep Awaiting requested review from shyuep shyuep is a code owner

@janosh janosh Awaiting requested review from janosh janosh is a code owner

@mkhorton mkhorton Awaiting requested review from mkhorton mkhorton is a code owner

Assignees
No one assigned
Labels
linting Linting and quality assurance types Type all the things
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@DanielYang59 @janosh