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

Caffeinate #13

Merged
merged 25 commits into from
Aug 21, 2024
Merged

Caffeinate #13

merged 25 commits into from
Aug 21, 2024

Conversation

ebanyas
Copy link
Collaborator

@ebanyas ebanyas commented Aug 15, 2024

Merging the "caffeinate" branch so we can adopt the updated build procedure down the line

pymatgen/io/espresso/caffeinate.py Outdated Show resolved Hide resolved
pymatgen/io/espresso/caffeinate.py Outdated Show resolved Hide resolved
pymatgen/io/espresso/caffeinate.py Outdated Show resolved Hide resolved
@ebanyas
Copy link
Collaborator Author

ebanyas commented Aug 17, 2024

Comments as of last commit:

  • _caffeinate_kpoints.py functions as expected on my test set of KPOINTS files
  • all cases feed to the same return statement
  • imports limited to specific cards
  • basic errors and warning classes added
  • KPointsSupportedModes comparison is now direct

pymatgen/io/espresso/cafe.py Outdated Show resolved Hide resolved
pymatgen/io/espresso/cafe.py Outdated Show resolved Hide resolved
pymatgen/io/espresso/cafe.py Outdated Show resolved Hide resolved
pymatgen/io/espresso/cafe.py Outdated Show resolved Hide resolved
pymatgen/io/espresso/cafe.py Outdated Show resolved Hide resolved
pymatgen/io/espresso/cafe.py Outdated Show resolved Hide resolved
pymatgen/io/espresso/cafe.py Outdated Show resolved Hide resolved
pymatgen/io/espresso/cafe.py Outdated Show resolved Hide resolved
pymatgen/io/espresso/inputs/pwin.py Outdated Show resolved Hide resolved
pymatgen/io/espresso/cafe.py Outdated Show resolved Hide resolved
@oashour
Copy link
Collaborator

oashour commented Aug 20, 2024

Although I love cafe.py, it's more pythonic to name that after the class caffeinator.py. I would add that as part of the inputs module, and you can add from pymatgen.io.espresso.inputs.caffeinator import Caffeinator as Caffeinator to inputs/__init__.py, so that users can just from pymatgen.io.espresso.inputs import Caffeinator. All other functions in that file are currently not part of the public API so they should not be accessible to users unless they want to dig in for them.

@ebanyas
Copy link
Collaborator Author

ebanyas commented Aug 20, 2024

Although I love cafe.py, it's more pythonic to name that after the class caffeinator.py. I would add that as part of the inputs module, and you can add from pymatgen.io.espresso.inputs.caffeinator import Caffeinator as Caffeinator to inputs/__init__.py, so that users can just from pymatgen.io.espresso.inputs import Caffeinator. All other functions in that file are currently not part of the public API so they should not be accessible to users unless they want to dig in for them.

I thought as of our last conversation, you wanted the caffeination features outside of inputs? They were originally within inputs; I moved them up one level after one of our slack conversations. I can move them back down if you want.

@oashour
Copy link
Collaborator

oashour commented Aug 20, 2024

I forgot our last conversation 😅. It's honestly not very important right now. You can keep it as an upper-level module caffeinator

Copy link
Collaborator

@oashour oashour left a comment

Choose a reason for hiding this comment

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

I think just these last few comments and we're good to go!

pymatgen/io/espresso/caffeinator.py Outdated Show resolved Hide resolved
pymatgen/io/espresso/caffeinator.py Outdated Show resolved Hide resolved
pymatgen/io/espresso/caffeinator.py Show resolved Hide resolved
pymatgen/io/espresso/caffeinator.py Outdated Show resolved Hide resolved
@oashour oashour merged commit 3b873f7 into main Aug 21, 2024
3 of 5 checks passed
@oashour
Copy link
Collaborator

oashour commented Aug 21, 2024

Merged! Thank you for your contribution, sous dev ✨

@ebanyas ebanyas deleted the caffeinate branch August 21, 2024 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants