Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Re-organizing VASP I/O code#3997

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
mkhorton wants to merge2 commits intomaster
base:master
Choose a base branch
Loading
fromvasp-split
Draft

Re-organizing VASP I/O code#3997

mkhorton wants to merge2 commits intomasterfromvasp-split

Conversation

@mkhorton
Copy link
Member

Summary

Re-opening changes initially introduced by@janosh in#3865 and later reverted by@shyuep (seethread).

I will be taking responsibility for this PR. This PR will require approval from both@janosh and@shyuep before merging.

Specifically,

  • For each module changed, I will manually inspect__dir__() to make sureall imports remain unaffected and that there are no backwards-incompatible changes.
  • Also splitpymatgen.io.vasp.inputs andpymatgen.io.vasp.outputs into separate files.

This change is well-motivated to improve maintainability.

I hope we can get this PR merged without too much difficulty. Thank you again to@janosh for starting this PR, to@Andrew-S-Rosen for the bug report due to the errors caused, and to@shyuep for implementing a rapid fix.

Andrew-S-Rosen, JaGeo, and janosh reacted with thumbs up emojiDanielYang59 reacted with rocket emoji
@mkhortonmkhorton changed the titleRe-organizingRe-organizing VASP I/O codeAug 12, 2024
@mkhortonmkhorton marked this pull request as draftAugust 12, 2024 22:30
@@ -0,0 +1,107 @@
# ruff: noqa: PGH003

This comment was marked as resolved.

@shyuep
Copy link
Member

shyuep commentedAug 13, 2024
edited
Loading

Here are my comments:

  1. Axiom 1: No. of lines is not a good metric of whether a module should be broken up. In even the most basic IDEs, there are tools to show the class structure, not to mention keyboard shortcuts to jump to any class/method name/line. I can't recall when the last time I had to use mouse scrolling for any file was. Note that the code files in vasp io are hardly the largest ones in pymatgen.
  2. Given Axiom 1, the only reason to break up a module into separate pieces is if there is alogical consistency to the subparts. While I can get onboard with breaking up sets.py (which has many classes), inputs.py and outputs.py should stay as they are. There is no good reason to split them up. Inputs only has 5 classes and they all pertain to vasp inputs.
  3. Even when breaking up sets.py, there is a certain inconsistency to the current choice. Why should input sets be split based on who wrote them, i.e., MP, MIT, MVL? Why not based on type of calculation (relaxation, static, electronic structure, NEB, etc.)? Do people select input sets based on organization? Isn't it more logical that they select an input set based on what they want to do?
  4. In so far as module subdivision is merely a developer-related detail (which it seems you are suggesting it is, since the primary justification was "ease of maintenance") and not a user API detail, the modules should all be private, i.e., with a "_" prefix. This gives the flexibility to completely reorganize when needed without some user seeing his codefrom pymatgen.vasp.io.sets.mp import MPRelaxSet suffer from backwards incompatible breakages. I do not want to see anyone using that line - it must befrom pymatgen.vasp.io.sets import MPRelaxSet, even if the sets reside in separate files.

I refer you to the scikit-learn module and class organization. Please use that as a guide on how to organize. Finally, I would point out that many of sklearn's module files are about the same length as sets.py (2800-3200 lines). They do not break up a module for maintenance - they break it up when there is a logic to the organization.

DanielYang59 reacted with thumbs up emoji

@mkhorton
Copy link
MemberAuthor

The sklearn comparison is illustrative: even where there are large LOC, often there is only a single class per file, and they are quite readable. This is not the case for these specific files in pymatgen. I am not making axiomatic statements; I agree that a high number of LOC is not in itself an error.

Long-time pymatgen developers (we are in good company) might know exactly where in the file to go to make an edit, but before you can get there, one has to read and understand the structure of the code. It is nice to know what classes are available, for example, without needing to read the entire file first.

Point 3 I concur; perhaps it is better to split up the input sets according to functionality instead.

JaGeo reacted with thumbs up emoji

@janosh
Copy link
Member

Re 3, i think the most common use case is to have consistent settings across whatever mixture of workflows you run (relax, band gap, EOS, phonon, etc.). so after having run some number of relaxations, a user is unlikely to care about half a dozen other band gap workflows with inconsistent VASP settings to their relaxations. more likely, the only thing they want to know is if a band gap workflow from the same lineage (say MP) exists. hence splitting input sets by lineage makes sense imo.

i guess it doesn't have to be one or the other. if you look atatomate2/vasp/jobs, it has a mixture of splits by lineage (mp.py,matpes.py,lobster.py,amset.py) and by workflow type (md.py,elastic.py,phonon.py, ...)

@shyuep
Copy link
Member

If splitting by source is preferred, I am ok with that. Note that I don't really think atomate2's organizational structure should be the reference here. In any case, if the module split is a private implementation, then it really doesn't matter since we can always reorganize as needed without breaking downstream codes.

JaGeo reacted with thumbs up emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@shyuepshyuepAwaiting requested review from shyuepshyuep is a code owner

@janoshjanoshAwaiting requested review from janosh

1 more reviewer

@DanielYang59DanielYang59DanielYang59 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mkhorton@shyuep@janosh@DanielYang59

[8]ページ先頭

©2009-2025 Movatter.jp