-
Notifications
You must be signed in to change notification settings - Fork 868
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 type annotations for core.(trajectory/units)
#3832
Merged
janosh
merged 18 commits into
materialsproject:master
from
DanielYang59:type-core-remain2-struct-unit-tracj
May 17, 2024
Merged
Improve type annotations for core.(trajectory/units)
#3832
janosh
merged 18 commits into
materialsproject:master
from
DanielYang59:type-core-remain2-struct-unit-tracj
May 17, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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>
DanielYang59
force-pushed
the
type-core-remain2-struct-unit-tracj
branch
from
May 16, 2024 12:36
43096cf
to
1683c18
Compare
DanielYang59
commented
May 17, 2024
DanielYang59
changed the title
Improve type annotations for core.(trajectory/units/structure)
Improve type annotations for core.(trajectory/units)
May 17, 2024
DanielYang59
changed the title
Improve type annotations for core.(trajectory/units)
Improve type annotations for May 17, 2024
core.(trajectory/units)
Can I have this one reviewed please? @janosh |
janosh
approved these changes
May 17, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @DanielYang59! 👍
Thanks for reviewing! Appreciate that. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
core.trajectory
andcore.units
_get_si_unit
and_check_mappings
inside the only methods that call them inUnit
Extend the following to support argument "unit" as both
str
andUnit
__init/new__
ofFloatWithUnit
andArrayWithUnit
get_conversion_factor
ofUnit
, andto
methods ofFloatWithUnit
andArrayWithUnit
The type hint suggests
str
, docstring suggestUnit
and implementation seems to handle partially of both.pymatgen/pymatgen/core/units.py
Lines 295 to 305 in bb68c78
Might be good to support both for more flexibility, without sacrificing backwards compatibility.