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

Use Self type in Method Signatures #3705

Merged
merged 84 commits into from
Mar 29, 2024

Conversation

DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented Mar 24, 2024

Summary

Use Self for return type in Method Signatures, see reference here.

Major changes:

  • Use Self type in Method Signatures
  • Add related type annotations
  • Some sourcery fixes

Minor changes:

  • Docstring tweaks

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features
    • Enhanced search functionality with more intuitive variable names and clearer method signatures.
    • Improved clarity and consistency in documentation across various modules.
  • Refactor
    • Updated method return types to leverage Self for better type hinting and future maintainability.
    • Modified method signatures for clarity and consistency.
  • Chores
    • Updated pre-commit hook configurations for improved code quality checks.
  • Style
    • Clarified and condensed docstrings for better readability.
  • Bug Fixes
    • Fixed potential issues with handling None values in data processing methods.

pymatgen/core/ion.py Outdated Show resolved Hide resolved
@janosh janosh added housekeeping Moving around or cleaning up old code/files types Type all the things labels Mar 24, 2024
@janosh
Copy link
Member

janosh commented Mar 24, 2024

i added Self return type to __new__ and __enter__ methods. created 66 mypy errors unfortunately so feel free to revert if we don't want to deal with this now

@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Mar 24, 2024

i added Self return type to __new__ and __enter__ methods. created 66 mypy errors unfortunately so feel free to revert if we don't want to deal with this now

Thanks for your help! And don't worry about the mypy errors (I already opened some magic boxes 😄 , don't mind adding a few more). I would try to fix as many as I can later.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Review Status

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 4bbee19 and 6d06bef.
Files selected for processing (4)
  • pymatgen/core/lattice.py (2 hunks)
  • pymatgen/io/vasp/inputs.py (25 hunks)
  • pymatgen/io/xyz.py (5 hunks)
  • pymatgen/io/zeopp.py (6 hunks)
Files skipped from review as they are similar to previous changes (4)
  • pymatgen/core/lattice.py
  • pymatgen/io/vasp/inputs.py
  • pymatgen/io/xyz.py
  • pymatgen/io/zeopp.py
Additional Context Used

@janosh janosh enabled auto-merge (squash) March 29, 2024 09:28
@janosh
Copy link
Member

janosh commented Mar 29, 2024

Looks like some internal error for the Windows runner.

very strange. did not notice this before in other PRs. if i see it a 2nd time I'll do some digging

@janosh janosh merged commit 6a06f3c into materialsproject:master Mar 29, 2024
22 checks passed
@DanielYang59 DanielYang59 deleted the use-self-type branch March 29, 2024 10:04
@DanielYang59
Copy link
Contributor Author

Cheers!

@janosh
Copy link
Member

janosh commented Mar 29, 2024

with all the type fixes in this PR, we can probably consider unskipping mypy in CI. i.e. remove this line

skip: [mypy]

@DanielYang59
Copy link
Contributor Author

we can probably consider unskipping mypy in CI

Ahh... Frankly I don't know how would this potentially alter (impact) CI. I thought mypy has always been running in CI?

@janosh
Copy link
Member

janosh commented Mar 29, 2024

I thought mypy has always been running in CI?

it has but not in pre-commit.ci, only in GitHub actions. for some reason (maybe different set of CLI args), the former reported more errors in the past which is why i skipped it. but i think we can try again now to see how many errors remain

@DanielYang59
Copy link
Contributor Author

Speaking of this. I might continue to add type annotations across the code base in the near future. But I guess it's better to start from the most used parts (core and vasp I guess?) instead of alphabetically, do you have any suggestions?

@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Mar 29, 2024

it has but not in pre-commit.ci, only in GitHub actions. for some reason (maybe different set of CLI args), the former reported more errors in the past which is why i skipped it. but i think we can try again now to see how many errors remain

Thanks for clarifying. This makes sense to me, but I guess it's better to run locally with the same args, instead of risk breaking everyone's PR workflow? I'm having a try now.

@janosh
Copy link
Member

janosh commented Mar 29, 2024

instead of risk breaking everyone's CI workflow?

oh don't worry, skip applies only to ci. mypy was only skipped in pre-commit.ci

Screenshot 2024-03-29 at 13 02 22

Speaking of this. I might continue to add type annotations across the code base in the near future. But I guess it's better to start from the most used parts (core and vasp I guess?) instead of alphabetically, do you have any suggestions?

that would be very welcome! core is definitely most important. vasp, electronic_structure, phonons are also important

@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Mar 29, 2024

I think I don't understand the difference between CI and pre-commit properly (currently doing some research on this). From my current understanding, pre-commit seems to check modified file for a PR only, right?

On the other hand, running mypy pymatgen gives:

Found 732 errors in 96 files (checked 278 source files)

................................ OMG

@janosh
Copy link
Member

janosh commented Mar 29, 2024

From my current understanding, pre-commit seems to check modified file for a PR only, right?

that's correct. you can still run it on all files with pre-commit run --all-files

On the other hand, running mypy pymatgen gives:

Found 732 errors in 96 files (checked 278 source files)

................................ OMG

😆 yep, lots of legacy issues to clean up. can't be done all at once. but some type errors could also go away with a smarter type checker in the future. hoping ruff becomes a type checker! 🤞

@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Mar 29, 2024

I'm hoping the same, but there are actually quite some issues can be discovered by type checking (mostly None as default type).

On the other hand, mypy --check-untyped-defs pymatgen gives:

Found 2080 errors in 159 files (checked 278 source files)

I wouldn't worry about this too much, we fixed like 120? in 5 days, I just want to duplicate myself...

@kavanase
Copy link
Contributor

Hi! Thanks for your work on pymatgen! 😃
Just to flag, this update seems to be causing some failures on our GitHub Actions tests (for ShakeNBreak and doped) with the latest pymatgen version. I'm not sure if it just needs typing_extensions to be added to the requirements?

Run pytest -vv -m "not mpl_image_compare" tests  # all non-plotting tests
============================= test session starts ==============================
platform linux -- Python 3.11.9, pytest-8.1.1, pluggy-1.4.0 -- /opt/hostedtoolcache/Python/3.11.9/x64/bin/python
cachedir: .pytest_cache
Matplotlib: 3.8.4
Freetype: 2.6.1
rootdir: /home/runner/work/ShakeNBreak/ShakeNBreak
configfile: pyproject.toml
plugins: mpl-0.1[7](https://github.com/SMTG-Bham/ShakeNBreak/actions/runs/8663869495/job/23758934155#step:6:8).0
collecting ... collected 121 items / 1 error / 29 deselected / 92 selected

==================================== ERRORS ====================================
___________________ ERROR collecting tests/test_analysis.py ____________________
ImportError while importing test module '/home/runner/work/ShakeNBreak/ShakeNBreak/tests/test_analysis.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/opt/hostedtoolcache/Python/3.11.9/x64/lib/python3.11/site-packages/_pytest/python.py:520: in importtestmodule
    mod = import_path(
/opt/hostedtoolcache/Python/3.11.9/x64/lib/python3.11/site-packages/_pytest/pathlib.py:5[8](https://github.com/SMTG-Bham/ShakeNBreak/actions/runs/8663869495/job/23758934155#step:6:9)4: in import_path
    importlib.import_module(module_name)
/opt/hostedtoolcache/Python/3.11.9/x64/lib/python3.11/importlib/__init__.py:126: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
<frozen importlib._bootstrap>:1204: in _gcd_import
    ???
<frozen importlib._bootstrap>:1176: in _find_and_load
    ???
<frozen importlib._bootstrap>:1147: in _find_and_load_unlocked
    ???
<frozen importlib._bootstrap>:6[9](https://github.com/SMTG-Bham/ShakeNBreak/actions/runs/8663869495/job/23758934155#step:6:10)0: in _load_unlocked
    ???
/opt/hostedtoolcache/Python/3.[11](https://github.com/SMTG-Bham/ShakeNBreak/actions/runs/8663869495/job/23758934155#step:6:12).9/x64/lib/python3.11/site-packages/_pytest/assertion/rewrite.py:178: in exec_module
    exec(co, module.__dict__)
tests/test_analysis.py:[14](https://github.com/SMTG-Bham/ShakeNBreak/actions/runs/8663869495/job/23758934155#step:6:15): in <module>
    from shakenbreak import analysis
shakenbreak/analysis.py:23: in <module>
    from shakenbreak import input, io
shakenbreak/input.py:30: in <module>
    from doped.generation import DefectsGenerator, name_defect_entries
/opt/hostedtoolcache/Python/3.11.9/x64/lib/python3.11/site-packages/doped/generation.py:32: in <module>
    from pymatgen.transformations.advanced_transformations import CubicSupercellTransformation
/opt/hostedtoolcache/Python/3.11.9/x64/lib/python3.11/site-packages/pymatgen/transformations/advanced_transformations.py:36: in <module>
    from pymatgen.transformations.standard_transformations import (
/opt/hostedtoolcache/Python/3.11.9/x64/lib/python3.11/site-packages/pymatgen/transformations/standard_transformations.py:[17](https://github.com/SMTG-Bham/ShakeNBreak/actions/runs/8663869495/job/23758934155#step:6:18): in <module>
    from pymatgen.analysis.elasticity.strain import Deformation
/opt/hostedtoolcache/Python/3.11.9/x64/lib/python3.11/site-packages/pymatgen/analysis/elasticity/__init__.py:5: in <module>
    from .elastic import (
/opt/hostedtoolcache/Python/3.11.9/x64/lib/python3.11/site-packages/pymatgen/analysis/elasticity/elastic.py:21: in <module>
    from pymatgen.analysis.elasticity.stress import Stress
/opt/hostedtoolcache/Python/3.11.9/x64/lib/python3.11/site-packages/pymatgen/analysis/elasticity/stress.py:11: in <module>
    from typing_extensions import Self
E   ModuleNotFoundError: No module named 'typing_extensions'

@janosh
Copy link
Member

janosh commented Apr 12, 2024

@kavanase thanks for reporting. those imports should have been guarded by a if TYPE_CHECKING: so as not to be executed at run time. luckily a simple fix in #3752. we'll make a hot fix release tomorrow.

@kavanase
Copy link
Contributor

Great, thanks @janosh!

@kavanase
Copy link
Contributor

Hi! Sorry to be bugging again 😅 but just flagging something that was causing a few test failures for us.
The format of Kpoints.kpts has been changed to be a list of tuples rather than a list of lists previously, but this change has only been made for certain cases, while others are still in list format.
Maybe this is intended (if we can update our tests to accept both), but I think it mightn't be, and best to have a consistent format?

@kavanase
Copy link
Contributor

kavanase commented Apr 13, 2024

6a06f3c#r140937732
6a06f3c#r140937733

(Linking to the comments I added for specific code locations as examples)

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.

3 participants