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

Breaking behavior: Removal of pymatgen.io.vasp.sets.get_vasprun_outcar without warning #3988

Closed
Andrew-S-Rosen opened this issue Aug 9, 2024 · 4 comments
Labels

Comments

@Andrew-S-Rosen
Copy link
Member

Andrew-S-Rosen commented Aug 9, 2024

Python version

3.11

Pymatgen version

pymatgen 2024.8.8

Operating system version

No response

Current behavior

from pymatgen.io.vasp.sets import get_vasprun_outcar

The above used to work on pymatgen < 2024.8.8 but does not exist any longer in pymatgen >= 2024.8.8. I don't believe there was a deprecation warning, so it may break other people's codes as-is.

Expected Behavior

A deprecation warning before removal.

Minimal example

No response

Relevant files to reproduce this bug

No response

@shyuep
Copy link
Member

shyuep commented Aug 9, 2024

This has been fixed with the 2024.8.9 emergency release.

@shyuep shyuep closed this as completed Aug 9, 2024
@Andrew-S-Rosen
Copy link
Member Author

Thank you!

@Andrew-S-Rosen Andrew-S-Rosen changed the title Breaking behavior: Removal of pymatgen.io.vasp.sets.get_vasp_outcar without warning Breaking behavior: Removal of pymatgen.io.vasp.sets.get_vasprun_outcar without warning Aug 10, 2024
@janosh
Copy link
Member

janosh commented Aug 11, 2024

@shyuep please unrevert my #3865 PR. just add back the
get_vasprun_outcar function that was accidentally dropped during merge conflict resolution which I might add was unnecessarily confusing because you moved everything to a src layout after i opened #3865. the reason it wasn't already merged before you pointlessly moved all those files (for which btw you never asked my or @mkhorton's approval either) is because you asked that i do it after #3835 which was sensible so i did.

the split of several-thousand-line files is an obvious improvement. you're throwing away my time by undoing that work

in #3989 you write

I did not agree to this.

what's that supposed to mean? there are hundreds of PRs that you don't agree to. if every one of those required your explicit approval, pymatgen development would grind to a standstill

@mkhorton
Copy link
Member

I think we should re-open #3865 and more comprehensively test.

It's a good idea to split these files up for maintainability. This can be done without backwards-incompatible changes; it seems like get_vasprun_outcar was an oversight.

However, I would go further than #3865. The pymatgen.io.vasp.outputs and pymatgen.io.vasp.inputs could also be split up into new folders without introducing backwards incompatibility, and just to improve ease-of-navigation. This would mean, e.g. pymatgen.io.vasp.inputs.potcar etc. Like the /src change, this sort of change does not need to introduce any breakage.

To be specific pymatgen.io.vasp.inputs is currently 2932 LOC, .outputs is 5363 LOC and .sets is 3205 LOC. While having long files is not in itself an error, it does makes things more difficult than necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants