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

Fix mypy issues in calculators and schema modules #2326

Merged
merged 29 commits into from
Jul 6, 2024

Conversation

AitElBadaoui
Copy link
Contributor

@AitElBadaoui AitElBadaoui commented Jul 3, 2024

Summary of Changes

Fixed mypy issues in the calculators and schema modules.

FYI: @Andrew-S-Rosen, we reduced the number of mypy errors from 320 to 160. There are still over 130 errors in types.py, which I will try to fix in another PR.

The remaining issues are linked to design problems such as:

  • The Atoms used in Quacc for the moment overload the ASE Atoms class with attributes and functions like charges, as_dict(), and from_dict(), which mypy does not accept. Should we ignore these, or create a QuaccAtom class with the necessary attributes and functions? what do you think ?

Requirements

Notes

@buildbot-princeton
Copy link
Collaborator

Can one of the admins verify this patch?

@AitElBadaoui AitElBadaoui changed the title Fix some mypy issue Fix mypy issues in calculators and schema modules Jul 3, 2024
Copy link

codecov bot commented Jul 3, 2024

Codecov Report

Attention: Patch coverage is 86.44068% with 8 lines in your changes missing coverage. Please review.

Project coverage is 98.86%. Comparing base (6b2ef31) to head (6087629).
Report is 1 commits behind head on main.

Files Patch % Lines
src/quacc/calculators/vasp/vasp.py 60.00% 2 Missing ⚠️
src/quacc/calculators/vasp/vasp_custodian.py 60.00% 2 Missing ⚠️
src/quacc/runners/ase.py 87.50% 1 Missing ⚠️
src/quacc/schemas/ase.py 80.00% 1 Missing ⚠️
src/quacc/utils/dicts.py 80.00% 1 Missing ⚠️
src/quacc/wflow_tools/customizers.py 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2326      +/-   ##
==========================================
- Coverage   99.06%   98.86%   -0.20%     
==========================================
  Files          85       85              
  Lines        3520     3533      +13     
==========================================
+ Hits         3487     3493       +6     
- Misses         33       40       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@Andrew-S-Rosen Andrew-S-Rosen left a comment

Choose a reason for hiding this comment

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

@AitElBadaoui: Thank you very much for your edits once again!

I have several questions below. Would you be able to address them when you get a chance? Again, most of these are genuine questions rather than critiques.

src/quacc/atoms/phonons.py Outdated Show resolved Hide resolved
src/quacc/calculators/espresso/espresso.py Outdated Show resolved Hide resolved
src/quacc/calculators/espresso/utils.py Show resolved Hide resolved
src/quacc/calculators/vasp/vasp.py Outdated Show resolved Hide resolved
src/quacc/runners/ase.py Outdated Show resolved Hide resolved
src/quacc/schemas/ase.py Outdated Show resolved Hide resolved
src/quacc/utils/dicts.py Outdated Show resolved Hide resolved
src/quacc/wflow_tools/decorators.py Outdated Show resolved Hide resolved
@Andrew-S-Rosen
Copy link
Member

The Atoms used in Quacc for the moment overload the ASE Atoms class with attributes and functions like charges, as_dict(), and from_dict(), which mypy does not accept. Should we ignore these, or create a QuaccAtom class with the necessary attributes and functions? what do you think ?

Interesting. I think this is a fair point. We do that patching here and perhaps a few other places. This is a technical necessity for some workflow tools and is very niche. For now, I would be okay with ignoring them to be honest. This whole business is coming from the fact that sometimes an Atoms object is being used and sometimes it is "secretly" an MSONAtoms object. Probably not worth digging into here, but... ignoring it seems wise for now.

@Andrew-S-Rosen
Copy link
Member

@buildbot-princeton test this please

@AitElBadaoui
Copy link
Contributor Author

Interesting. I think this is a fair point. We do that patching here and perhaps a few other places. This is a technical necessity for some workflow tools and is very niche. For now, I would be okay with ignoring them to be honest. This whole business is coming from the fact that sometimes an Atoms object is being used and sometimes it is "secretly" an MSONAtoms object. Probably not worth digging into here, but... ignoring it seems wise for now.

Sounds good. I've already ignored this type of error.

@Andrew-S-Rosen
Copy link
Member

@buildbot-princeton test this please

@Andrew-S-Rosen
Copy link
Member

Thank you, @AitElBadaoui!! This should be good to go. I will merge later this evening :)

@Andrew-S-Rosen Andrew-S-Rosen merged commit a1620a9 into Quantum-Accelerators:main Jul 6, 2024
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants