BoothGroup / Vayesta

A Python package for wave function-based quantum embedding
Apache License 2.0
33 stars 8 forks source link

Import cleaning #96

Closed obackhouse closed 1 year ago

obackhouse commented 1 year ago

Improves handling of optional dependencies including on installation, and will hopefully remove relative and star imports once I am done

obackhouse commented 1 year ago

Also waiting on #92 so I can make sure ebcc is handled consistently

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 96.33% and project coverage change: -0.16 :warning:

Comparison is base (ed2f91f) 72.12% compared to head (079885a) 71.97%.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #96 +/- ## ========================================== - Coverage 72.12% 71.97% -0.16% ========================================== Files 138 138 Lines 18476 18402 -74 Branches 2578 2578 ========================================== - Hits 13325 13244 -81 - Misses 4420 4426 +6 - Partials 731 732 +1 ``` | [Impacted Files](https://app.codecov.io/gh/BoothGroup/Vayesta/pull/96?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BoothGroup) | Coverage Δ | | |---|---|---| | [vayesta/core/symmetry/symmetry.py](https://app.codecov.io/gh/BoothGroup/Vayesta/pull/96?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BoothGroup#diff-dmF5ZXN0YS9jb3JlL3N5bW1ldHJ5L3N5bW1ldHJ5LnB5) | `0.00% <0.00%> (ø)` | | | [vayesta/core/symmetry/tsymmetry.py](https://app.codecov.io/gh/BoothGroup/Vayesta/pull/96?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BoothGroup#diff-dmF5ZXN0YS9jb3JlL3N5bW1ldHJ5L3RzeW1tZXRyeS5weQ==) | `0.00% <ø> (ø)` | | | [vayesta/core/types/ebwf.py](https://app.codecov.io/gh/BoothGroup/Vayesta/pull/96?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BoothGroup#diff-dmF5ZXN0YS9jb3JlL3R5cGVzL2Vid2YucHk=) | `0.00% <0.00%> (ø)` | | | [vayesta/core/types/orbitals.py](https://app.codecov.io/gh/BoothGroup/Vayesta/pull/96?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BoothGroup#diff-dmF5ZXN0YS9jb3JlL3R5cGVzL29yYml0YWxzLnB5) | `72.78% <ø> (-0.37%)` | :arrow_down: | | [vayesta/core/types/wf/ccsdtq.py](https://app.codecov.io/gh/BoothGroup/Vayesta/pull/96?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BoothGroup#diff-dmF5ZXN0YS9jb3JlL3R5cGVzL3dmL2Njc2R0cS5weQ==) | `82.14% <ø> (-1.73%)` | :arrow_down: | | [vayesta/core/types/wf/cisdtq.py](https://app.codecov.io/gh/BoothGroup/Vayesta/pull/96?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BoothGroup#diff-dmF5ZXN0YS9jb3JlL3R5cGVzL3dmL2Npc2R0cS5weQ==) | `89.36% <ø> (-0.64%)` | :arrow_down: | | [vayesta/core/util.py](https://app.codecov.io/gh/BoothGroup/Vayesta/pull/96?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BoothGroup#diff-dmF5ZXN0YS9jb3JlL3V0aWwucHk=) | `74.70% <ø> (-0.66%)` | :arrow_down: | | [vayesta/core/vlog.py](https://app.codecov.io/gh/BoothGroup/Vayesta/pull/96?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BoothGroup#diff-dmF5ZXN0YS9jb3JlL3Zsb2cucHk=) | `81.10% <ø> (-0.15%)` | :arrow_down: | | [vayesta/edmet/uedmet.py](https://app.codecov.io/gh/BoothGroup/Vayesta/pull/96?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BoothGroup#diff-dmF5ZXN0YS9lZG1ldC91ZWRtZXQucHk=) | `100.00% <ø> (ø)` | | | [vayesta/ewf/helper.py](https://app.codecov.io/gh/BoothGroup/Vayesta/pull/96?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BoothGroup#diff-dmF5ZXN0YS9ld2YvaGVscGVyLnB5) | `0.00% <0.00%> (-26.32%)` | :arrow_down: | | ... and [85 more](https://app.codecov.io/gh/BoothGroup/Vayesta/pull/96?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BoothGroup) | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

maxnus commented 1 year ago

I would say relative imports are OK as long as they are explicit (with a leading .) and a limited number of star imports are OK, if the imported module defines __all__. What's the reason to get rid of them?

obackhouse commented 1 year ago

PEP 8 recommends against relative imports:

Relative imports for intra-package imports are highly discouraged. Always use the absolute package path for all imports.

Absolute imports make it easier to avoid circular importing issues in complex packages.

As for star imports, PEP 8 does not explicitly mention them as far as I know, however in my opinion (and I think it's a common opinion) they're a bad idea. Not only do they pollute the namespace of the module, but they make it extremely difficult to chase down the origin of imported classes and methods, and generally reduce readability. They also prevent some static analysis and flaking tools on top of that.

I think in both cases, given that you can achieve the same behaviour with explicit and absolute imports, the latter is just the better option.

ghb24 commented 1 year ago

I certainly agree that following an execution thread of someone else's code is more difficult with star imports. But maybe thats because I'm still not using an IDE...

maxnus commented 1 year ago

@obackhouse Where did you find this quote in PEP 8? Was it maybe a Python 2 version, before we had the dot-syntax? https://peps.python.org/pep-0008/#imports says

However, explicit relative imports are an acceptable alternative to absolute imports, especially when dealing with complex package layouts where using absolute imports would be unnecessarily verbose

I don't have strong feelings about the star import, I agree they should generally be avoided. Side note @ghb24 I now switched to PyCharm after so many years of vim it's the best thing ever :grin: (just install ideavim to keep the commands)

obackhouse commented 1 year ago

@maxnus yes maybe you're right, that's outdated PEP 8. That being said, any decent linter or formatter will force absolute imports, since they're more readable (except for the fact they're longer, I guess). I guess that doesn't bode well for my renewed plans to format Vayesta...

If you have strong feelings, I can revert the relative imports.

cjcscott commented 1 year ago

I think there is a decent benefit to consistency in our code, and I would err on the side of absolute imports just for clarity's sake. While relative imports are definitely sometimes easier to write, given the plethora of tools to allow automatic conversion between relative imports and absolute imports (including in PyCharm, which I'm glad @maxnus has finally come around to using!).

obackhouse commented 1 year ago

@maxnus I've just removed all the unused imports in https://github.com/BoothGroup/Vayesta/pull/96/commits/f5426de0866bd9d6c9a49bda185a40cee5074b69, I excluded __init__.py files - can you think of any other instances that there may be unused imports that are desired or does this sound OK?

ghb24 commented 1 year ago

@obackhouse : Conflicts now with updated master...

obackhouse commented 1 year ago

@obackhouse : Conflicts now with updated master...

I'll sort it, don't think I'm done just yet anyway

obackhouse commented 1 year ago

Good to merge now